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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM! Do you want to make a similar commit to update the tests? I was there as a discussion on Discord, so please wait a bit before committing in case @ldionne wants to chime in.



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


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


================
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 \
----------------
Can you commit the changes to this file in a separate commit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119430