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

[BUILD] Fix multiple assignment operators for SpinLockMutex #2501

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

SaiHarshaK
Copy link
Contributor

@SaiHarshaK SaiHarshaK commented Jan 22, 2024

Fixes #2500

Changes

Please provide a brief description of the changes here.
The multiple assignment operators for SpinLockMutex throws warnings in consumer build for these headers. This would require them to explicitly add #Pragma to ignore C4522.

This PR removes the redundant assignment to let the build pass without these warnings. Tested locally as I was able to repro this issue before this change.

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

@SaiHarshaK SaiHarshaK requested a review from a team January 22, 2024 16:54
Copy link

linux-foundation-easycla bot commented Jan 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@ThomsonTan
Copy link
Contributor

@SaiHarshaK thanks for the contribution. Please take a look and sign the EasyCLA.

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 quick PR.

I would rather remove the volatile declaration instead,
and keep the regular one.

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8f88553) 87.09% compared to head (2bbf0c0) 87.09%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2501   +/-   ##
=======================================
  Coverage   87.09%   87.09%           
=======================================
  Files         200      200           
  Lines        6103     6103           
=======================================
  Hits         5315     5315           
  Misses        788      788           
Files Coverage Δ
api/include/opentelemetry/common/spin_lock_mutex.h 25.00% <ø> (ø)

@SaiHarshaK
Copy link
Contributor Author

@marcalff I wasn't sure if there was any specific use case around why volatile was added in the first place, so assumed that might be the safer one to add.
I can certainly update this PR to use the other one.

@ThomsonTan
Copy link
Contributor

Both declarations are marking the assignment operator as deleted instead of defining it. I am still not quite sure the reason of the triggered warning.

@SaiHarshaK SaiHarshaK force-pushed the u/skottapalli/fix_warns branch from 1df6dd1 to 217a945 Compare January 22, 2024 18:01
@SaiHarshaK
Copy link
Contributor Author

Had to repush for clearing EasyCLA

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.

@ThomsonTan
Copy link
Contributor

I didn't repro the warning with MSVC 19.38.33134, and also both overloads look correct to me, so could it be an issue for the specific MSVC @SaiHarshaK is using?

@SaiHarshaK
Copy link
Contributor Author

Perhaps so, I don't have a way to currently update and check the same, since the infra on which build runs is out of my control.

@ThomsonTan ThomsonTan merged commit fe15d0a into open-telemetry:main Jan 23, 2024
48 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.

Warning: Multiple assignment operators specified for SpinLockMutex
4 participants