Skip to content

Support [[msvc::no_unique_address]] with cl. #19

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

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

lukester1975
Copy link
Contributor

Hi

Very nice library!

Was wondering if you would consider supporting [[msvc::no_unique_address]] with cl? [[no_unique_address]] is silently ignored.

Also, clang-cl noisily complains about both: https://reviews.llvm.org/D110485

Something like this works for me - though maybe making it configurable is preferable, to avoid ABI breakage?

Regards

Luke.

@codeinred
Copy link
Owner

Thank you so much for your PR! On line 11 you don't provide a definition for TUPLET_NO_UNIQUE_ADDRESS - is this the issue you were referring to with clang complaining about it on windows?

* [[no_unique_address]] with cl is silently ignored.
* Don't use either with with clang-cl, they are unrecognised.

Useful links:

* https://reviews.llvm.org/D110485
* llvm/llvm-project#49358
@lukester1975
Copy link
Contributor Author

Yeah sadly there are two cases:

  • cl silently ignores [[no_unique_address]] but supports their own [[msvc::no_unique_address]]. It seems like the next breaking ABI version will enable real [[no_unique_address]] support, from what I've read.
  • clang-cl nosily ignores both (with "unknown attribute" warnings). It appears they will be adding support for the msvc specific attribute "Now that Microsoft has decided to define [[msvc::no_unique_address]] with a particular ABI, it would be reasonable for us to support that spelling" (from the link above), but it's not there yet!

I've reworked the PR to use __has_cpp_attribute so as and when support is added it should just be correct. Note that __has_cpp_attribute(msvc::no_unique_address) itself is currently wrong hence the additional old-style check for (cl && !clang)...

@lukester1975
Copy link
Contributor Author

So that commit was just to quieten the warnings - I didn't want to use -Wno-missing-attributes (too coarse) and clang-cl doesn't support the pragma to disable only the [[no_unique_address]] warning.

Anyway, I though I'd check what effect [[msvc::no_unique_address] has on tuplet's layout - and the answer was very surprising (to me, anyway!). I've added another commit on this branch with some tests if you're interested, but in summary it doesn't allow an element's alignment padding to be used for the next element, irrespective of whether it's an aggregate or not.

So there's no reduction in tuple size but at least the warnings are gone...

@codeinred codeinred merged commit 553562c into codeinred:main Feb 11, 2022
@codeinred
Copy link
Owner

Hi! I'm really sorry it took me so long to get back to you! School was pretty busy this week. I've merged your changes! I appreciate your help with the project!

@lukester1975 lukester1975 deleted the cl-support branch February 11, 2022 09:04
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.

2 participants