[libc++] Add `explicit` to a bunch of private detail ctors.

[• Quuxplusone]

Summary

Notably the following ctors remain non-explicit because they
are used as implicit conversions in too many places:
* __debug_less(_Compare&)
* __map_iterator(_TreeIterator)
* __map_const_iterator(_TreeIterator)
* __hash_map_iterator(_HashIterator)
* __hash_map_const_iterator(_HashIterator)

This has been on my to-do list for a while, just for general code cleanliness. This was not done with any mechanical assistance (e.g. a compiler warning for non-explicit ctors); it was just me knowing about certain ctors for a long time (e.g. __bit_reference), and then brainstorming or randomly happening across some others.

Oddly, only Clang (not GCC or MSVC) thinks that this change has beneficial effects on overload resolution. I'm not sure who's correct here, but, C++ being C++, I would guess that GCC and MSVC are correct and Clang is wrong. https://godbolt.org/z/K76sr5MG9

[ldionne]

LGTM in essence, but the patch itself has a few mistakes (I think).

libcxx/include/__memory/compressed_pair.h

44

This looks like a merge conflict to me -- you seem to have undone part of a recent patch by @philnik. Reintroducing INLINE_VISIBILITY and changing the template parameters.

[• Quuxplusone]

Fix merge conflict; add __deque_iterator and a few more.

[Mordante]

LGTM!

libcxx/include/__debug

196

Just curious, but is there a good reason to make a default constructor explicit?

[• Quuxplusone]

libcxx/include/__debug

196

Just the usual: it prevents implicit conversions.(*)

• explicit Foo(int, int) prevents void f(Foo); f({1,1});
• explicit Foo() prevents void f(Foo); f({});

My personal rule is "Make every ctor explicit unless you intend it to be called implicitly (which should be rare because implicit conversions are confusing)." The exceptions of course are the copy and move ctors, because we do intend them to be called implicitly all over the place.

(* - pedantic note: the "conversion" from a braced initializer list is not technically a conversion, because the braced initializer list doesn't have a type to begin with. But it's the same idea, both to a human who thinks of {1,1} as "a pair of ints" and to the compiler who thinks of it as a context requiring a non-explicit ctor.)

libcxx/include/__bit_reference

#endif
     __bit_reference& operator=(const __bit_reference& __x) _NOEXCEPT
         {return operator=(static_cast<bool>(__x));}
 
     _LIBCPP_INLINE_VISIBILITY void flip() _NOEXCEPT {*__seg_ ^= __mask_;}
     _LIBCPP_INLINE_VISIBILITY __bit_iterator<_Cp, false> operator&() const _NOEXCEPT
         {return __bit_iterator<_Cp, false>(__seg_, static_cast<unsigned>(__libcpp_ctz(__mask_)));}
 private:
     _LIBCPP_INLINE_VISIBILITY
-    __bit_reference(__storage_pointer __s, __storage_type __m) _NOEXCEPT
+    explicit __bit_reference(__storage_pointer __s, __storage_type __m) _NOEXCEPT
         : __seg_(__s), __mask_(__m) {}
 };
 
 template <class _Cp>
 class __bit_reference<_Cp, false>
 {
 };
 
public:
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR operator bool() const _NOEXCEPT
         {return static_cast<bool>(*__seg_ & __mask_);}
 
     _LIBCPP_INLINE_VISIBILITY __bit_iterator<_Cp, true> operator&() const _NOEXCEPT
         {return __bit_iterator<_Cp, true>(__seg_, static_cast<unsigned>(__libcpp_ctz(__mask_)));}
 private:
     _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR
-    __bit_const_reference(__storage_pointer __s, __storage_type __m) _NOEXCEPT
+    explicit __bit_const_reference(__storage_pointer __s, __storage_type __m) _NOEXCEPT
         : __seg_(__s), __mask_(__m) {}
 
     __bit_const_reference& operator=(const __bit_const_reference&) = delete;
 };
 
 // find
 
 template <class _Cp, bool _IsConst>
#endif
     _LIBCPP_INLINE_VISIBILITY friend bool operator<=(const __bit_iterator& __x, const __bit_iterator& __y)
         {return !(__y < __x);}
 
     _LIBCPP_INLINE_VISIBILITY friend bool operator>=(const __bit_iterator& __x, const __bit_iterator& __y)
         {return !(__x < __y);}
 
 private:
     _LIBCPP_INLINE_VISIBILITY
-    __bit_iterator(__storage_pointer __s, unsigned __ctz) _NOEXCEPT
+    explicit __bit_iterator(__storage_pointer __s, unsigned __ctz) _NOEXCEPT
         : __seg_(__s), __ctz_(__ctz) {}
 
     friend typename _Cp::__self;
 
     friend class __bit_reference<_Cp>;
     friend class __bit_const_reference<_Cp>;
     friend class __bit_iterator<_Cp, true>;
     template <class _Dp> friend struct __bit_array;

libcxx/include/__debug

#ifndef _LIBCPP_CXX03_LANG
     __c_node& operator=(const __c_node&) = delete;
 #else
 private:
     __c_node(const __c_node&);
     __c_node& operator=(const __c_node&);
 public:
 #endif
     _LIBCPP_INLINE_VISIBILITY
-    __c_node(void* __c, __c_node* __next)
+    explicit __c_node(void* __c, __c_node* __next)
         : __c_(__c), __next_(__next), beg_(nullptr), end_(nullptr), cap_(nullptr) {}
     virtual ~__c_node();
 
     virtual bool __dereferenceable(const void*) const = 0;
     virtual bool __decrementable(const void*) const = 0;
     virtual bool __addable(const void*, ptrdiff_t) const = 0;
     virtual bool __subscriptable(const void*, ptrdiff_t) const = 0;
 
     void __add(__i_node* __i);
     _LIBCPP_HIDDEN void __remove(__i_node* __i);
 };
 
 template <class _Cont>
 struct _C_node
     : public __c_node
 {
-    _C_node(void* __c, __c_node* __n)
+    explicit _C_node(void* __c, __c_node* __n)
         : __c_node(__c, __n) {}
 
     virtual bool __dereferenceable(const void*) const;
     virtual bool __decrementable(const void*) const;
     virtual bool __addable(const void*, ptrdiff_t) const;
     virtual bool __subscriptable(const void*, ptrdiff_t) const;
 };
 

 {
     __c_node** __cbeg_;
     __c_node** __cend_;
     size_t   __csz_;
     __i_node** __ibeg_;
     __i_node** __iend_;
     size_t   __isz_;
 
-    __libcpp_db();
+    explicit __libcpp_db();
 public:
 #ifndef _LIBCPP_CXX03_LANG
     __libcpp_db(const __libcpp_db&) = delete;
     __libcpp_db& operator=(const __libcpp_db&) = delete;
 #else
 private:
     __libcpp_db(const __libcpp_db&);
     __libcpp_db& operator=(const __libcpp_db&);

libcxx/include/__iterator/common_iterator.h

 template<input_or_output_iterator _Iter, sentinel_for<_Iter> _Sent>
   requires (!same_as<_Iter, _Sent> && copyable<_Iter>)
 class common_iterator {
   class __proxy {
     friend common_iterator;
 
     iter_value_t<_Iter> __value;
     // We can move __x because the only caller verifies that __x is not a reference.
-    constexpr __proxy(iter_reference_t<_Iter>&& __x)
+    constexpr explicit __proxy(iter_reference_t<_Iter>&& __x)
       : __value(_VSTD::move(__x)) {}
 
   public:
     constexpr const iter_value_t<_Iter>* operator->() const noexcept {
       return _VSTD::addressof(__value);
     }
   };
 
   class __postfix_proxy {
     friend common_iterator;
 
     iter_value_t<_Iter> __value;
-    constexpr __postfix_proxy(iter_reference_t<_Iter>&& __x)
+    constexpr explicit __postfix_proxy(iter_reference_t<_Iter>&& __x)
       : __value(_VSTD::forward<iter_reference_t<_Iter>>(__x)) {}
 
   public:
     constexpr const iter_value_t<_Iter>& operator*() const noexcept {
       return __value;
     }
   };
 

libcxx/include/__iterator/istreambuf_iterator.h

public:
     typedef basic_istream<_CharT,_Traits>   istream_type;
 private:
     mutable streambuf_type* __sbuf_;
 
     class __proxy
     {
         char_type __keep_;
         streambuf_type* __sbuf_;
-        _LIBCPP_INLINE_VISIBILITY __proxy(char_type __c, streambuf_type* __s)
+        _LIBCPP_INLINE_VISIBILITY
+        explicit __proxy(char_type __c, streambuf_type* __s)
             : __keep_(__c), __sbuf_(__s) {}
         friend class istreambuf_iterator;
     public:
         _LIBCPP_INLINE_VISIBILITY char_type operator*() const {return __keep_;}
     };
 
     _LIBCPP_INLINE_VISIBILITY
     bool __test_for_eof() const

libcxx/include/__memory/compressed_pair.h

 struct __value_init_tag {};
 
 template <class _Tp, int _Idx, bool _CanBeEmptyBase = is_empty<_Tp>::value && !__libcpp_is_final<_Tp>::value>
 struct __compressed_pair_elem {
   using _ParamT = _Tp;
   using reference = _Tp&;
   using const_reference = const _Tp&;
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair_elem(__default_init_tag) {}
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair_elem(__value_init_tag) : __value_() {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(__default_init_tag) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(__value_init_tag) : __value_() {}
 
   template <class _Up, class = __enable_if_t<!is_same<__compressed_pair_elem, typename decay<_Up>::type>::value> >
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit
-  __compressed_pair_elem(_Up&& __u) : __value_(std::forward<_Up>(__u)) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
+  explicit __compressed_pair_elem(_Up&& __u) : __value_(std::forward<_Up>(__u)) {}
 
 #ifndef _LIBCPP_CXX03_LANG
-  template <class... _Args, size_t... _Indices>
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
-  __compressed_pair_elem(piecewise_construct_t, tuple<_Args...> __args, __tuple_indices<_Indices...>)
-      : __value_(std::forward<_Args>(std::get<_Indices>(__args))...) {}
+  template <class... _Args, size_t... _Indexes>
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
+  explicit __compressed_pair_elem(piecewise_construct_t, tuple<_Args...> __args,
+                         __tuple_indices<_Indexes...>)
+      : __value_(_VSTD::forward<_Args>(_VSTD::get<_Indexes>(__args))...) {}
 #endif
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 reference __get() _NOEXCEPT { return __value_; }
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR const_reference __get() const _NOEXCEPT { return __value_; }
 
 private:
   _Tp __value_;
 };
 
 template <class _Tp, int _Idx>
 struct __compressed_pair_elem<_Tp, _Idx, true> : private _Tp {
   using _ParamT = _Tp;
   using reference = _Tp&;
   using const_reference = const _Tp&;
   using __value_type = _Tp;
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair_elem() = default;
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair_elem(__default_init_tag) {}
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair_elem(__value_init_tag) : __value_type() {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem() = default;
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(__default_init_tag) {}
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(__value_init_tag) : __value_type() {}
 
   template <class _Up, class = __enable_if_t<!is_same<__compressed_pair_elem, typename decay<_Up>::type>::value> >
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
   explicit __compressed_pair_elem(_Up&& __u) : __value_type(std::forward<_Up>(__u)) {}
 
 #ifndef _LIBCPP_CXX03_LANG
   template <class... _Args, size_t... _Indices>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
public:
 
   template <bool _Dummy = true,
     class = __enable_if_t<
         __dependent_type<is_default_constructible<_T1>, _Dummy>::value &&
         __dependent_type<is_default_constructible<_T2>, _Dummy>::value
     >
   >
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
-  __compressed_pair() : _Base1(__value_init_tag()), _Base2(__value_init_tag()) {}
+  explicit __compressed_pair() : _Base1(__value_init_tag()), _Base2(__value_init_tag()) {}
 
   template <class _U1, class _U2>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
-  __compressed_pair(_U1&& __t1, _U2&& __t2) : _Base1(std::forward<_U1>(__t1)), _Base2(std::forward<_U2>(__t2)) {}
+  explicit __compressed_pair(_U1&& __t1, _U2&& __t2) : _Base1(std::forward<_U1>(__t1)), _Base2(std::forward<_U2>(__t2)) {}
 
 #ifndef _LIBCPP_CXX03_LANG
   template <class... _Args1, class... _Args2>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
-  __compressed_pair(piecewise_construct_t __pc, tuple<_Args1...> __first_args,
+  explicit __compressed_pair(piecewise_construct_t __pc, tuple<_Args1...> __first_args,
                              tuple<_Args2...> __second_args)
       : _Base1(__pc, std::move(__first_args), typename __make_tuple_indices<sizeof...(_Args1)>::type()),
         _Base2(__pc, std::move(__second_args), typename __make_tuple_indices<sizeof...(_Args2)>::type()) {}
 #endif
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
   typename _Base1::reference first() _NOEXCEPT {
     return static_cast<_Base1&>(*this).__get();
   }

libcxx/include/iomanip

 }
 
 
 template <class _CharT, class _Traits, class _Iter>
 basic_ostream<_CharT, _Traits>& operator<<(
          basic_ostream<_CharT, _Traits>& __os,
          const __quoted_output_proxy<_CharT, _Iter, _Traits> & __proxy)
 {
-    return __quoted_output (__os, __proxy.__first, __proxy.__last, __proxy.__delim, __proxy.__escape);
+    return __quoted_output(__os, __proxy.__first, __proxy.__last, __proxy.__delim, __proxy.__escape);
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 struct __quoted_proxy
 {
     basic_string<_CharT, _Traits, _Allocator> &__string;
     _CharT  __delim;
     _CharT  __escape;
 
-    __quoted_proxy(basic_string<_CharT, _Traits, _Allocator> &__s, _CharT __d, _CharT __e)
+    explicit __quoted_proxy(basic_string<_CharT, _Traits, _Allocator> &__s, _CharT __d, _CharT __e)
         : __string(__s), __delim(__d), __escape(__e) {}
 };
 
 template <class _CharT, class _Traits, class _Allocator>
 _LIBCPP_INLINE_VISIBILITY
 basic_ostream<_CharT, _Traits>& operator<<(
         basic_ostream<_CharT, _Traits>& __os,
         const __quoted_proxy<_CharT, _Traits, _Allocator> & __proxy)
 {