[libcxx] Wrap [[no_unique_address]] in a macro, for clang-cl

[mstorsjo]

Summary

This, on top of D119134, allows enabling -Werror for the clang-cl
configurations.

[philnik]

LGTM % comments

libcxx/include/__config

1404

1411

libcxx/include/__ranges/join_view.h

72

I'd personally keep [[no_unique_address]] here.

[mstorsjo]

With this (and D119134) I think we could enable LIBCXX_ENABLE_WERROR in all CI configurations except for the couple GCC ones.

GCC builds seem to print (at least) this warning when building:

include/c++/v1/string:2267:32: error: enumerated and non-enumerated type in conditional expression [-Werror=extra]
 2267 |   size_type __cap = __is_short ? __min_cap : __get_long_cap();

[mstorsjo]

libcxx/include/__ranges/join_view.h

72

Oh, oops. Yes this is a victim of search-and-replace...

[mstorsjo]

Updated according to the suggestions.

[philnik]

In D119430#3310901, @mstorsjo wrote:

With this (and D119134) I think we could enable LIBCXX_ENABLE_WERROR in all CI configurations except for the couple GCC ones.

GCC builds seem to print (at least) this warning when building:

include/c++/v1/string:2267:32: error: enumerated and non-enumerated type in conditional expression [-Werror=extra]
 2267 |   size_type __cap = __is_short ? __min_cap : __get_long_cap();

I'm working on making the GCC build warning-free, but I don't know how long it will take. There are still quite a few problems.

[Mordante]

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.

libcxx/include/__config

1408

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.

1417

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.

libcxx/utils/ci/run-buildbot

573 ↗

(On Diff #407497)

Can you commit the changes to this file in a separate commit?

[mstorsjo]

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

libcxx/include/__config

1408

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.

1417

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.

libcxx/utils/ci/run-buildbot

573 ↗

(On Diff #407497)

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.

[• Quuxplusone]

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

libcxx/include/__config

1408

+1

1417

Please remove the /* nothing */ comment, as we don't do that anywhere else AFAIK.

libcxx/include/__ranges/join_view.h

72

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)

[Mordante]

In D119430#3311864, @mstorsjo wrote:

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

libcxx/include/__config

1417

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

1417

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.

[mstorsjo]

Reordered the preference between [[msvc::no_unique_address]] and [[no_unique_address]] as requestd, adjusted the comments based on that

[Mordante]

In D119430#3312381, @mstorsjo wrote:

Reordered the preference between [[msvc::no_unique_address]] and [[no_unique_address]] as requestd, adjusted the comments based on that

Thanks! Still LGTM!

[jloser]

libcxx/include/__ranges/join_view.h

72

@Quuxplusone thanks for the ping here. I certainly don't know why the comment is here as you know. I'd be OK removing it (and also marking __base_ as _LIBCPP_NO_UNIQUE_ADDRESS FWIW - effectively what I did in https://reviews.llvm.org/D119208)

[ldionne]

LGTM, and the CI failures are unrelated.

libcxx/include/__algorithm/in_in_out_result.h

 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
 namespace ranges {
 
 template <class _I1, class _I2, class _O1>
 struct in_in_out_result {
-  [[no_unique_address]] _I1 in1;
-  [[no_unique_address]] _I2 in2;
-  [[no_unique_address]] _O1 out;
+  _LIBCPP_NO_UNIQUE_ADDRESS _I1 in1;
+  _LIBCPP_NO_UNIQUE_ADDRESS _I2 in2;
+  _LIBCPP_NO_UNIQUE_ADDRESS _O1 out;
 
   template <class _II1, class _II2, class _OO1>
     requires convertible_to<const _I1&, _II1> && convertible_to<const _I2&, _II2> && convertible_to<const _O1&, _OO1>
   _LIBCPP_HIDE_FROM_ABI constexpr
   operator in_in_out_result<_II1, _II2, _OO1>() const& {
     return {in1, in2, out};
   }
 

libcxx/include/__algorithm/in_in_result.h

 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
 namespace ranges {
 
 template <class _I1, class _I2>
 struct in_in_result {
-  [[no_unique_address]] _I1 in1;
-  [[no_unique_address]] _I2 in2;
+  _LIBCPP_NO_UNIQUE_ADDRESS _I1 in1;
+  _LIBCPP_NO_UNIQUE_ADDRESS _I2 in2;
 
   template <class _II1, class _II2>
     requires convertible_to<const _I1&, _II1> && convertible_to<const _I2&, _II2>
    _LIBCPP_HIDE_FROM_ABI constexpr
    operator in_in_result<_II1, _II2>() const & {
     return {in1, in2};
   }
 

libcxx/include/__algorithm/in_out_out_result.h

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #ifndef _LIBCPP_HAS_NO_CONCEPTS
 
 namespace ranges {
 template <class _I1, class _O1, class _O2>
 struct in_out_out_result {
-  [[no_unique_address]] _I1 in;
-  [[no_unique_address]] _O1 out1;
-  [[no_unique_address]] _O2 out2;
+  _LIBCPP_NO_UNIQUE_ADDRESS _I1 in;
+  _LIBCPP_NO_UNIQUE_ADDRESS _O1 out1;
+  _LIBCPP_NO_UNIQUE_ADDRESS _O2 out2;
 
   template <class _II1, class _OO1, class _OO2>
     requires convertible_to<const _I1&, _II1> && convertible_to<const _O1&, _OO1> && convertible_to<const _O2&, _OO2>
   _LIBCPP_HIDE_FROM_ABI constexpr
   operator in_out_out_result<_II1, _OO1, _OO2>() const& {
     return {in, out1, out2};
   }
 

libcxx/include/__algorithm/in_out_result.h

 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
 namespace ranges {
 
 template<class _InputIterator, class _OutputIterator>
 struct in_out_result {
-  [[no_unique_address]] _InputIterator in;
-  [[no_unique_address]] _OutputIterator out;
+  _LIBCPP_NO_UNIQUE_ADDRESS _InputIterator in;
+  _LIBCPP_NO_UNIQUE_ADDRESS _OutputIterator out;
 
   template <class _InputIterator2, class _OutputIterator2>
     requires convertible_to<const _InputIterator&, _InputIterator2> && convertible_to<const _OutputIterator&,
                            _OutputIterator2>
   _LIBCPP_HIDE_FROM_ABI
   constexpr operator in_out_result<_InputIterator2, _OutputIterator2>() const & {
     return {in, out};
   }

libcxx/include/__config

#if defined(__GNUC__) || defined(__clang__)
   // The attribute uses 1-based indices for ordinary and static member functions.
   // The attribute uses 2-based indices for non-static member functions.
 # define _LIBCPP_ATTRIBUTE_FORMAT(archetype, format_string_index, first_format_arg_index) \
     __attribute__((__format__(archetype, format_string_index, first_format_arg_index)))
 #else
 # define _LIBCPP_ATTRIBUTE_FORMAT(archetype, format_string_index, first_format_arg_index) /* nothing */
 #endif
 
+#if __has_cpp_attribute(msvc::no_unique_address)
+   // MSVC implements [[no_unique_address]] as a silent no-op currently.
+   // (If/when MSVC breaks its C++ ABI, it will be changed to work as intended.)
+   // However, MSVC implements [[msvc::no_unique_address]] which does what
+   // [[no_unique_address]] is supposed to do, in general.
+
+   // Clang-cl does not yet (14.0) implement either [[no_unique_address]] or
+   // [[msvc::no_unique_address]] though. If/when it does implement
+   // [[msvc::no_unique_address]], this should be preferred though.
+#  define _LIBCPP_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
+#elif __has_cpp_attribute(no_unique_address)
+#  define _LIBCPP_NO_UNIQUE_ADDRESS [[no_unique_address]]
+#else
+#  define _LIBCPP_NO_UNIQUE_ADDRESS /* nothing */
+   // Note that this can be replaced by #error as soon as clang-cl
+   // 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.
+   // We geenrally don't want to use this macro outside of C++20-only code,
+   // because using it conditionally in one language version only would make
+   // the ABI inconsistent.
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

libcxx/include/__iterator/counted_iterator.h

 
 template<input_or_output_iterator _Iter>
 class counted_iterator
   : public __counted_iterator_concept<_Iter>
   , public __counted_iterator_category<_Iter>
   , public __counted_iterator_value_type<_Iter>
 {
 public:
-  [[no_unique_address]] _Iter __current_ = _Iter();
+  _LIBCPP_NO_UNIQUE_ADDRESS _Iter __current_ = _Iter();
   iter_difference_t<_Iter> __count_ = 0;
 
   using iterator_type = _Iter;
   using difference_type = iter_difference_t<_Iter>;
 
   _LIBCPP_HIDE_FROM_ABI
   constexpr counted_iterator() requires default_initializable<_Iter> = default;
 

libcxx/include/__ranges/copyable_box.h

 
 template<class _Tp>
 concept __copy_constructible_object = copy_constructible<_Tp> && is_object_v<_Tp>;
 
 namespace ranges {
   // Primary template - uses std::optional and introduces an empty state in case assignment fails.
   template<__copy_constructible_object _Tp>
   class __copyable_box {
-    [[no_unique_address]] optional<_Tp> __val_;
+    _LIBCPP_NO_UNIQUE_ADDRESS optional<_Tp> __val_;
 
   public:
     template<class ..._Args>
       requires is_constructible_v<_Tp, _Args...>
     _LIBCPP_HIDE_FROM_ABI
     constexpr explicit __copyable_box(in_place_t, _Args&& ...__args)
       noexcept(is_nothrow_constructible_v<_Tp, _Args...>)
       : __val_(in_place, _VSTD::forward<_Args>(__args)...)
namespace ranges {
   concept __doesnt_need_empty_state_for_copy = copyable<_Tp> || is_nothrow_copy_constructible_v<_Tp>;
 
   template<class _Tp>
   concept __doesnt_need_empty_state_for_move = movable<_Tp> || is_nothrow_move_constructible_v<_Tp>;
 
   template<__copy_constructible_object _Tp>
     requires __doesnt_need_empty_state_for_copy<_Tp> && __doesnt_need_empty_state_for_move<_Tp>
   class __copyable_box<_Tp> {
-    [[no_unique_address]] _Tp __val_;
+    _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
 
   public:
     template<class ..._Args>
       requires is_constructible_v<_Tp, _Args...>
     _LIBCPP_HIDE_FROM_ABI
     constexpr explicit __copyable_box(in_place_t, _Args&& ...__args)
       noexcept(is_nothrow_constructible_v<_Tp, _Args...>)
       : __val_(_VSTD::forward<_Args>(__args)...)

libcxx/include/__ranges/drop_view.h

namespace ranges {
   {
     // We cache begin() whenever ranges::next is not guaranteed O(1) to provide an
     // amortized O(1) begin() method. If this is an input_range, then we cannot cache
     // begin because begin is not equality preserving.
     // Note: drop_view<input-range>::begin() is still trivially amortized O(1) because
     // one can't call begin() on it more than once.
     static constexpr bool _UseCache = forward_range<_View> && !(random_access_range<_View> && sized_range<_View>);
     using _Cache = _If<_UseCache, __non_propagating_cache<iterator_t<_View>>, __empty_cache>;
-    [[no_unique_address]] _Cache __cached_begin_ = _Cache();
+    _LIBCPP_NO_UNIQUE_ADDRESS _Cache __cached_begin_ = _Cache();
     range_difference_t<_View> __count_ = 0;
     _View __base_ = _View();
 
 public:
     drop_view() requires default_initializable<_View> = default;
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr drop_view(_View __base, range_difference_t<_View> __count)

libcxx/include/__ranges/join_view.h

namespace ranges {
   private:
     using _InnerRange = range_reference_t<_View>;
 
     template<bool> struct __iterator;
     template<bool> struct __sentinel;
 
     static constexpr bool _UseCache = !is_reference_v<_InnerRange>;
     using _Cache = _If<_UseCache, __non_propagating_cache<remove_cvref_t<_InnerRange>>, __empty_cache>;
-    [[no_unique_address]] _Cache __cache_;
+    _LIBCPP_NO_UNIQUE_ADDRESS _Cache __cache_;
     _View __base_ = _View(); // TODO: [[no_unique_address]] makes clang crash! File a bug :)
 
   public:
     _LIBCPP_HIDE_FROM_ABI
     join_view() requires default_initializable<_View> = default;
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr explicit join_view(_View __base)
       : __base_(_VSTD::move(__base)) {}
 

libcxx/include/__ranges/reverse_view.h

 namespace ranges {
   template<view _View>
     requires bidirectional_range<_View>
   class reverse_view : public view_interface<reverse_view<_View>> {
     // We cache begin() whenever ranges::next is not guaranteed O(1) to provide an
     // amortized O(1) begin() method.
     static constexpr bool _UseCache = !random_access_range<_View> && !common_range<_View>;
     using _Cache = _If<_UseCache, __non_propagating_cache<reverse_iterator<iterator_t<_View>>>, __empty_cache>;
-    [[no_unique_address]] _Cache __cached_begin_ = _Cache();
-    [[no_unique_address]] _View __base_ = _View();
+    _LIBCPP_NO_UNIQUE_ADDRESS _Cache __cached_begin_ = _Cache();
+    _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
 
   public:
     _LIBCPP_HIDE_FROM_ABI
     reverse_view() requires default_initializable<_View> = default;
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr explicit reverse_view(_View __view) : __base_(_VSTD::move(__view)) {}
 

libcxx/include/__ranges/subrange.h

namespace ranges {
   class _LIBCPP_TEMPLATE_VIS subrange
     : public view_interface<subrange<_Iter, _Sent, _Kind>>
   {
   private:
     static constexpr bool _StoreSize = (_Kind == subrange_kind::sized && !sized_sentinel_for<_Sent, _Iter>);
     static constexpr bool _MustProvideSizeAtConstruction = !_StoreSize; // just to improve compiler diagnostics
     struct _Empty { constexpr _Empty(auto) noexcept { } };
     using _Size = conditional_t<_StoreSize, make_unsigned_t<iter_difference_t<_Iter>>, _Empty>;
-    [[no_unique_address]] _Iter __begin_ = _Iter();
-    [[no_unique_address]] _Sent __end_ = _Sent();
-    [[no_unique_address]] _Size __size_ = 0;
+    _LIBCPP_NO_UNIQUE_ADDRESS _Iter __begin_ = _Iter();
+    _LIBCPP_NO_UNIQUE_ADDRESS _Sent __end_ = _Sent();
+    _LIBCPP_NO_UNIQUE_ADDRESS _Size __size_ = 0;
 
   public:
     _LIBCPP_HIDE_FROM_ABI
     subrange() requires default_initializable<_Iter> = default;
 
     _LIBCPP_HIDE_FROM_ABI
     constexpr subrange(__convertible_to_non_slicing<_Iter> auto __iter, _Sent __sent)
       requires _MustProvideSizeAtConstruction

libcxx/include/__ranges/take_view.h

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
 namespace ranges {
   template<view _View>
   class take_view : public view_interface<take_view<_View>> {
-    [[no_unique_address]] _View __base_ = _View();
+    _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
     range_difference_t<_View> __count_ = 0;
 
     template<bool> class __sentinel;
 
   public:
     _LIBCPP_HIDE_FROM_ABI
     take_view() requires default_initializable<_View> = default;
 
namespace ranges {
   };
 
   template<view _View>
   template<bool _Const>
   class take_view<_View>::__sentinel {
     using _Base = __maybe_const<_Const, _View>;
     template<bool _OtherConst>
     using _Iter = counted_iterator<iterator_t<__maybe_const<_OtherConst, _View>>>;
-    [[no_unique_address]] sentinel_t<_Base> __end_ = sentinel_t<_Base>();
+    _LIBCPP_NO_UNIQUE_ADDRESS sentinel_t<_Base> __end_ = sentinel_t<_Base>();
 
     template<bool>
     friend class take_view<_View>::__sentinel;
 
 public:
     _LIBCPP_HIDE_FROM_ABI
     __sentinel() = default;
 

libcxx/include/__ranges/transform_view.h

concept __transform_view_constraints =
   __can_reference<invoke_result_t<_Fn&, range_reference_t<_View>>>;
 
 template<input_range _View, copy_constructible _Fn>
   requires __transform_view_constraints<_View, _Fn>
 class transform_view : public view_interface<transform_view<_View, _Fn>> {
   template<bool> class __iterator;
   template<bool> class __sentinel;
 
-  [[no_unique_address]] __copyable_box<_Fn> __func_;
-  [[no_unique_address]] _View __base_ = _View();
+  _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Fn> __func_;
+  _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
 
 public:
   _LIBCPP_HIDE_FROM_ABI
   transform_view()
     requires default_initializable<_View> && default_initializable<_Fn> = default;
 
   _LIBCPP_HIDE_FROM_ABI
   constexpr transform_view(_View __base, _Fn __func)