[PATCH] D48616: Implement LWG 2946, 3075 and 3076

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 5 inline comments as done.
mclow.lists added inline comments.


================
Comment at: include/string:836
         _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
-        basic_string(const _Tp& __t, size_type __pos, size_type __n,
-                     const allocator_type& __a = allocator_type(),
-                     typename enable_if<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value, void>::type* = 0);
-    _LIBCPP_INLINE_VISIBILITY explicit
-    basic_string(__self_view __sv);
-    _LIBCPP_INLINE_VISIBILITY
-    basic_string(__self_view __sv, const _Allocator& __a);
+        explicit basic_string(const _Tp& __t, size_type __pos, size_type __n,
+                              const allocator_type& __a = allocator_type());
----------------
ldionne wrote:
> I believe an unwelcome `explicit` snuck in here.
Right.


================
Comment at: include/string:839
+
+//     template<class _Tp>
+//         _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
----------------
ldionne wrote:
> Did you mean to leave those comments there?
Nope. Thanks.


================
Comment at: include/string:856
+        _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+        explicit basic_string(const _Tp& __t, const allocator_type& __a);
+
----------------
ldionne wrote:
> tcanens wrote:
> > ldionne wrote:
> > > I think this `explicit` shouldn't be there, too.
> > This one is `explicit` in the standard (because it had a default argument: `template<class T> explicit basic_string(const T& t, const Allocator& a = Allocator());`)
> Ah, you're right. So basically
> 
> ```
> template<class T>
> explicit basic_string(const T& t,
>                       const Allocator& a = Allocator());
> ```
> 
> is implemented as two overloads
> 
> ```
> template<class T>
> explicit basic_string(const T& t);
> ```
> 
> and 
> 
> ```
> template<class T>
> explicit basic_string(const T& t, const Allocator& a);
> ```
> 
Right. We frequently split defaulted allocator ctors.



================
Comment at: include/string:1987
 {
     __self_view __sv = __self_view(__t).substr(__pos, __n);
----------------
tcanens wrote:
> `__self_view(__t)` is wrong - the wording was intentionally crafted to require the conversion to `basic_string_view` to be done using copy-initialization. Using direct-initialization can potentially result in different overload resolution results.
Nice catch!


https://reviews.llvm.org/D48616