Skip to content

Conversation

@Reneg973
Copy link
Contributor

see #3713 (comment)

Fixes # (issue)

Changes

prevent self assignment and self move assignment

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@Reneg973 Reneg973 requested a review from a team as a code owner November 26, 2025 04:45
@Reneg973
Copy link
Contributor Author

Reneg973 commented Nov 26, 2025

Should I remove the unit test not belonging to this PR? It increases test coverage but I'm not sure whether you want to see it in a different commit!

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.93%. Comparing base (c19bfb6) to head (542b2cc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
api/include/opentelemetry/nostd/shared_ptr.h 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3768      +/-   ##
==========================================
- Coverage   89.95%   89.93%   -0.01%     
==========================================
  Files         225      225              
  Lines        7158     7165       +7     
==========================================
+ Hits         6438     6443       +5     
- Misses        720      722       +2     
Files with missing lines Coverage Δ
api/include/opentelemetry/nostd/shared_ptr.h 96.88% <50.00%> (-3.12%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marcalff
Copy link
Member

Should I remove the unit test not belonging to this PR? It increases test coverage but I'm not sure whether you want to see it in a different commit!

Please keep the test, this is fine.

It needs to build in CI, there is a self move warning. Maybe use another variable, like:

ptr4 = std::move(ptr1);

@lalitb
Copy link
Member

lalitb commented Nov 26, 2025

Should I remove the unit test not belonging to this PR? It increases test coverage but I'm not sure whether you want to see it in a different commit!

Please keep the test, this is fine.

It needs to build in CI, there is a self move warning. Maybe use another variable, like:

ptr4 = std::move(ptr1);

Or if we need to test self-assignment for shared pointer.

ptr1 = ptr1; 

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 once the CI is fixed.

@marcalff marcalff changed the title fix shared_ptr self assignments [API] fix shared_ptr self assignments Nov 26, 2025
@Reneg973
Copy link
Contributor Author

It needs to build in CI, there is a self move warning. Maybe use another variable, like:

Another variable won't work, I just could use an alias (reference to ptr1), but I'm not sure it tricks all the modern compilers. It may work for now, but not anymore after compiler update. Could also be fixed by suppressing this compiler warning temporarily (if allowed).
Any tips?

@marcalff
Copy link
Member

Any tips?

Sure, just remove the code in the unit tests that does not build, due to the self move warning.

The operator= change in the header is simple enough, it does not absolutely require unit test coverage if this brings complications.

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.

LGTM, thanks for the fix.

@marcalff marcalff merged commit 6678a09 into open-telemetry:main Dec 1, 2025
67 checks passed
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.

3 participants