[libcxx-commits] [PATCH] D119430: [libcxx] Wrap [[no_unique_address]] in a macro, for clang-cl

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D119430#3311864 <https://reviews.llvm.org/D119430#3311864>, @mstorsjo wrote:

> In D119430#3311673 <https://reviews.llvm.org/D119430#3311673>, @Mordante wrote:
>
>> Do you want to make a similar commit to update the tests?
>
> Hmm, is this used in tests somewhere too? (Haven’t grepped there yet, on the phone right now.) Those are already built with `-Werror` iirc, so they should have caused an issue already, but I can look closer a bit later.

I knew I had seen it, seems there's only one occurrence. Some other tests will probably start to fail in the near future since they contains `XFAIL: msvc` due to the lack of `[[unique_address]]` support.



================
Comment at: libcxx/include/__config:1417
+   // implements msvc::no_unique_address, since there should be no C++20
+   // compiler that doesn't support one of the two attributes at that point.
+#endif
----------------
Quuxplusone wrote:
> mstorsjo wrote:
> > Mordante wrote:
> > > I'm not sure we want to make this an error in the future. In general we don't do that so we can use these macros in older language standards.
> > No strings opinion on that - but AFAIK we only use this in C++20-only contexts anyway, as we’d have an inconsistent ABI otherwise.
> Please remove the `/* nothing */` comment, as we don't do that anywhere else AFAIK.
@mstorsjo good point regarding the ABI, maybe add that in the comment as justification why we want to make in an `#error` after clang-cl support the attribute.

@Quuxplusone I considered the same comment, but line 1398 disproves the statement ;-) `_LIBCPP_EXTERN_TEMPLATE` also has nothings.




================
Comment at: libcxx/include/__config:1417
+   // implements msvc::no_unique_address, since there should be no C++20
+   // compiler that doesn't support one of the two attributes at that point.
+#endif
----------------
Mordante wrote:
> Quuxplusone wrote:
> > mstorsjo wrote:
> > > Mordante wrote:
> > > > I'm not sure we want to make this an error in the future. In general we don't do that so we can use these macros in older language standards.
> > > No strings opinion on that - but AFAIK we only use this in C++20-only contexts anyway, as we’d have an inconsistent ABI otherwise.
> > Please remove the `/* nothing */` comment, as we don't do that anywhere else AFAIK.
> @mstorsjo good point regarding the ABI, maybe add that in the comment as justification why we want to make in an `#error` after clang-cl support the attribute.
> 
> @Quuxplusone I considered the same comment, but line 1398 disproves the statement ;-) `_LIBCPP_EXTERN_TEMPLATE` also has nothings.
> 
> 
> No strings opinion on that - but AFAIK we only use this in C++20-only contexts anyway, as we’d have an inconsistent ABI otherwise.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119430/new/

https://reviews.llvm.org/D119430