Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

why nostd::unique_ptr<Scope> not Scope #787

Closed
halayli opened this issue May 22, 2021 · 6 comments · Fixed by #788
Closed

why nostd::unique_ptr<Scope> not Scope #787

halayli opened this issue May 22, 2021 · 6 comments · Fixed by #788
Assignees
Labels
area:api release:required-for-ga To be resolved before GA release
Milestone

Comments

@halayli
Copy link

halayli commented May 22, 2021

Any reason why WithActiveSpan returns nostd::unique_ptr<Scope> instead of simply returning Scope?

@lalitb
Copy link
Member

lalitb commented May 23, 2021

Scope object manages the ongoing span context, and context unwinding happens when this object goes out of scope. It's important to have only one copy of this object, so it's wrapped under nostd::unique_ptr.

Also, if it's meant to be a question and not an issue we can move it to Discussions

@halayli
Copy link
Author

halayli commented May 23, 2021

I totally get the purpose of a span's Scope and its integration with the Context's internal Stack. But I don't understand why it needs to be allocated on the heap. It's a performance penalty that can be avoided, IMO. A scope class with deleted copy constructors should be enough. In fact for this particular case, they are implicitly deleted because it has a std::unique_ptr member variable.

It's definitely a question and not an issue. Sorry, I forgot about the discussion feature. Feel free to move it there.

@halayli halayli closed this as completed May 23, 2021
@lalitb lalitb reopened this May 23, 2021
@lalitb
Copy link
Member

lalitb commented May 23, 2021

We can keep it open for now in case it's a potential issue :) For now, will let @pyohannes reply at his work time as he originally implemented this. Else, will look into it.

@halayli
Copy link
Author

halayli commented May 23, 2021

Sounds great, thanks!

@lalitb
Copy link
Member

lalitb commented May 23, 2021

@halayli @pyohannes - Have raised a PR #788 with changes suggested in the issue. Can you please review them once?

@halayli
Copy link
Author

halayli commented May 23, 2021

Thanks for taking care of it! Yep, this should do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api release:required-for-ga To be resolved before GA release
Projects
None yet
2 participants