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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: jloser, Quuxplusone.
Quuxplusone accepted this revision.
Quuxplusone added a comment.

LGTM too, although we should understand whether the CI failures are related, and @ldionne should probably be aware of this new macro.



================
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
----------------
mstorsjo wrote:
> 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.
+1


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


================
Comment at: libcxx/include/__ranges/join_view.h:71
+    _LIBCPP_NO_UNIQUE_ADDRESS _Cache __cache_;
+    _View __base_ = _View(); // TODO: _LIBCPP_NO_UNIQUE_ADDRESS makes clang crash! File a bug :)
 
----------------
mstorsjo wrote:
> philnik wrote:
> > I'd personally keep `[[no_unique_address]]` here.
> Oh, oops. Yes this is a victim of search-and-replace...
Nobody knows why this comment is here; we should just remove it. But D119208 is related (and attn @jloser, for the merge conflict that will inevitably occur here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119430