[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

47

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

144

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

[• Quuxplusone]

libcxx/include/__debug

144

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

struct _LIBCPP_TYPE_VIS __c_node
     __i_node** beg_;
     __i_node** end_;
     __i_node** cap_;
 
     __c_node(const __c_node&) = delete;
     __c_node& operator=(const __c_node&) = delete;
 
     _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:
     __libcpp_db(const __libcpp_db&) = delete;
     __libcpp_db& operator=(const __libcpp_db&) = delete;
 
     ~__libcpp_db();
 
     class __db_c_iterator;
     class __db_c_const_iterator;

libcxx/include/__hash_table

#endif // _LIBCPP_DEBUG_LEVEL == 2
     }
     friend _LIBCPP_INLINE_VISIBILITY
     bool operator!=(const __hash_iterator& __x, const __hash_iterator& __y)
         {return !(__x == __y);}
 
 private:
 #if _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_INLINE_VISIBILITY
-    __hash_iterator(__next_pointer __node, const void* __c) _NOEXCEPT
+    explicit __hash_iterator(__next_pointer __node, const void* __c) _NOEXCEPT
         : __node_(__node)
         {
             __get_db()->__insert_ic(this, __c);
         }
 #else
     _LIBCPP_INLINE_VISIBILITY
-    __hash_iterator(__next_pointer __node) _NOEXCEPT
+    explicit __hash_iterator(__next_pointer __node) _NOEXCEPT
         : __node_(__node)
         {}
 #endif
     template <class, class, class, class> friend class __hash_table;
     template <class> friend class _LIBCPP_TEMPLATE_VIS __hash_const_iterator;
     template <class> friend class _LIBCPP_TEMPLATE_VIS __hash_map_iterator;
     template <class, class, class, class, class> friend class _LIBCPP_TEMPLATE_VIS unordered_map;
     template <class, class, class, class, class> friend class _LIBCPP_TEMPLATE_VIS unordered_multimap;
#endif // _LIBCPP_DEBUG_LEVEL == 2
     }
     friend _LIBCPP_INLINE_VISIBILITY
     bool operator!=(const __hash_const_iterator& __x, const __hash_const_iterator& __y)
         {return !(__x == __y);}
 
 private:
 #if _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_INLINE_VISIBILITY
-    __hash_const_iterator(__next_pointer __node, const void* __c) _NOEXCEPT
+    explicit __hash_const_iterator(__next_pointer __node, const void* __c) _NOEXCEPT
         : __node_(__node)
         {
             __get_db()->__insert_ic(this, __c);
         }
 #else
     _LIBCPP_INLINE_VISIBILITY
-    __hash_const_iterator(__next_pointer __node) _NOEXCEPT
+    explicit __hash_const_iterator(__next_pointer __node) _NOEXCEPT
         : __node_(__node)
         {}
 #endif
     template <class, class, class, class> friend class __hash_table;
     template <class> friend class _LIBCPP_TEMPLATE_VIS __hash_map_const_iterator;
     template <class, class, class, class, class> friend class _LIBCPP_TEMPLATE_VIS unordered_map;
     template <class, class, class, class, class> friend class _LIBCPP_TEMPLATE_VIS unordered_multimap;
 };
#endif // _LIBCPP_DEBUG_LEVEL == 2
     }
     friend _LIBCPP_INLINE_VISIBILITY
     bool operator!=(const __hash_local_iterator& __x, const __hash_local_iterator& __y)
         {return !(__x == __y);}
 
 private:
 #if _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_INLINE_VISIBILITY
-    __hash_local_iterator(__next_pointer __node, size_t __bucket,
+    explicit __hash_local_iterator(__next_pointer __node, size_t __bucket,
                                    size_t __bucket_count, const void* __c) _NOEXCEPT
         : __node_(__node),
           __bucket_(__bucket),
           __bucket_count_(__bucket_count)
         {
             __get_db()->__insert_ic(this, __c);
             if (__node_ != nullptr)
                 __node_ = __node_->__next_;
         }
 #else
     _LIBCPP_INLINE_VISIBILITY
-    __hash_local_iterator(__next_pointer __node, size_t __bucket,
+    explicit __hash_local_iterator(__next_pointer __node, size_t __bucket,
                                    size_t __bucket_count) _NOEXCEPT
         : __node_(__node),
           __bucket_(__bucket),
           __bucket_count_(__bucket_count)
         {
             if (__node_ != nullptr)
                 __node_ = __node_->__next_;
         }
 #endif
#endif // _LIBCPP_DEBUG_LEVEL == 2
     }
     friend _LIBCPP_INLINE_VISIBILITY
     bool operator!=(const __hash_const_local_iterator& __x, const __hash_const_local_iterator& __y)
         {return !(__x == __y);}
 
 private:
 #if _LIBCPP_DEBUG_LEVEL == 2
     _LIBCPP_INLINE_VISIBILITY
-    __hash_const_local_iterator(__next_pointer __node_ptr, size_t __bucket,
+    explicit __hash_const_local_iterator(__next_pointer __node_ptr, size_t __bucket,
                                          size_t __bucket_count, const void* __c) _NOEXCEPT
         : __node_(__node_ptr),
           __bucket_(__bucket),
           __bucket_count_(__bucket_count)
         {
             __get_db()->__insert_ic(this, __c);
             if (__node_ != nullptr)
                 __node_ = __node_->__next_;
         }
 #else
     _LIBCPP_INLINE_VISIBILITY
-    __hash_const_local_iterator(__next_pointer __node_ptr, size_t __bucket,
+    explicit __hash_const_local_iterator(__next_pointer __node_ptr, size_t __bucket,
                                          size_t __bucket_count) _NOEXCEPT
         : __node_(__node_ptr),
           __bucket_(__bucket),
           __bucket_count_(__bucket_count)
         {
             if (__node_ != nullptr)
                 __node_ = __node_->__next_;
         }
 #endif

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...>)
+  explicit __compressed_pair_elem(piecewise_construct_t, tuple<_Args...> __args, __tuple_indices<_Indices...>)
       : __value_(std::forward<_Args>(std::get<_Indices>(__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/__string

 template <class _CharT, class _Iter, class _Traits=char_traits<_CharT> >
 struct __quoted_output_proxy
 {
     _Iter  __first;
     _Iter  __last;
     _CharT  __delim;
     _CharT  __escape;
 
-    __quoted_output_proxy(_Iter __f, _Iter __l, _CharT __d, _CharT __e)
+    explicit __quoted_output_proxy(_Iter __f, _Iter __l, _CharT __d, _CharT __e)
         : __first(__f), __last(__l), __delim(__d), __escape(__e) {}
     //  This would be a nice place for a string_ref
 };
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___STRING

libcxx/include/deque

_LIBCPP_INLINE_VISIBILITY friend
         bool operator<=(const __deque_iterator& __x, const __deque_iterator& __y)
         {return !(__y < __x);}
 
     _LIBCPP_INLINE_VISIBILITY friend
         bool operator>=(const __deque_iterator& __x, const __deque_iterator& __y)
         {return !(__x < __y);}
 
 private:
-    _LIBCPP_INLINE_VISIBILITY __deque_iterator(__map_iterator __m, pointer __p) _NOEXCEPT
+    _LIBCPP_INLINE_VISIBILITY explicit __deque_iterator(__map_iterator __m, pointer __p) _NOEXCEPT
         : __m_iter_(__m), __ptr_(__p) {}
 
     template <class _Tp, class _Ap> friend class __deque_base;
     template <class _Tp, class _Ap> friend class _LIBCPP_TEMPLATE_VIS deque;
     template <class _Vp, class _Pp, class _Rp, class _MP, class _Dp, _Dp>
         friend class _LIBCPP_TEMPLATE_VIS __deque_iterator;
 
     template <class _RAIter,

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)
 {