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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

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.



================
Comment at: libcxx/include/__config:1408
+#  define _LIBCPP_NO_UNIQUE_ADDRESS [[no_unique_address]]
+#elif __has_cpp_attribute(msvc::no_unique_address)
+   // MSVC does implement [[msvc::no_unique_address]] which does what
----------------
Mordante wrote:
> Would it make sense to test this first? Then line 1401 doesn't need `_MSC_VER` and comment is only needed for `msvc::no_unique_address`.
I think that could work too. Then I think we’d still be warning free, as `__has_cpp_attribute()` should indicate that it isn’t available in clang-cl now.


================
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:
> 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.


================
Comment at: libcxx/utils/ci/run-buildbot:573
     # setting when cmake and the test driver does the right thing automatically.
     generate-cmake-libcxx-win -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF \
+                              -DLIBCXX_ENABLE_WERROR=ON \
----------------
Mordante wrote:
> Can you commit the changes to this file in a separate commit?
Yeah, it probably makes more sense to split out that too a separate commit - just bundled it here to show that we do get these configs warning free.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119430