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

Fix type mismatch when move nostd::shared_ptr #1815

Merged
merged 3 commits into from
Nov 29, 2022

Conversation

owent
Copy link
Member

@owent owent commented Nov 28, 2022

Signed-off-by: owent admin@owent.net

Fixes #1806

Changes

Fix type mismatch when move nostd::shared_ptr.

@owent owent requested a review from a team November 28, 2022 11:50
@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #1815 (1c93d3a) into main (33d244a) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1815   +/-   ##
=======================================
  Coverage   85.73%   85.73%           
=======================================
  Files         171      171           
  Lines        5240     5240           
=======================================
  Hits         4492     4492           
  Misses        748      748           
Impacted Files Coverage Δ
api/include/opentelemetry/nostd/shared_ptr.h 100.00% <100.00%> (ø)

Signed-off-by: owent <admin@owent.net>
Signed-off-by: owent <admin@owent.net>
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Validated the changes, there is no crash with the fix. Thanks.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

I don't even understand the original std::shared_ptr code in the first place,
and the fix even less.

Some explanations would be welcome, to understand what is going on here.

@lalitb lalitb merged commit 7e26a7e into open-telemetry:main Nov 29, 2022
@owent
Copy link
Member Author

owent commented Nov 30, 2022

Thanks for the fix.

I don't even understand the original std::shared_ptr code in the first place,
and the fix even less.

Some explanations would be welcome, to understand what is going on here.

If we move a nostd::shared_ptr of derived class to a nostd::shared_ptr of base class,we will use the constructor of std::shared_ptr<derived class> in the buffer of nostd::shared_ptr<base class>.And when we use this nostd::shared_ptr<base class>, the buffer will be reinpreter cast to std::shared_ptr<base class>* before.

@marcalff marcalff mentioned this pull request Dec 1, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
@owent owent deleted the fix_1806 branch December 8, 2022 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in shared_ptr<> when compiled with -O3
5 participants