[PATCH] D38075: Fix PR34668 - P0704R1 implementation is too permissive

[Tim Song via Phabricator via cfe-commits]

tcanens added a comment.

In https://reviews.llvm.org/D38075#1161325, @Rakete wrote:

> Do you need someone to commit it?


Yes, please.


https://reviews.llvm.org/D38075



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: include/string:856
+_LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
+explicit basic_string(const _Tp& __t, const allocator_type& __a);
+

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 explicit basic_string(const T& t, const Allocator& a = 
Allocator());`)



Comment at: include/string:1987
 {
 __self_view __sv = __self_view(__t).substr(__pos, __n);

`__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.


https://reviews.llvm.org/D48616



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43773: Implement the container bits of P0805R1

[Tim Song via Phabricator via cfe-commits]

tcanens added a comment.

Hmm, for `vector` and `deque`, we define a temporary variable, check that sizes 
match and then use range-and-a-half `equal`:

  const typename vector<_Tp1, _Allocator1>::size_type __sz = __x.size();
  return __sz == __y.size() && _VSTD::equal(__x.begin(), __x.end(), 
__y.begin());

For `list` we check that sizes match and then use range-and-a-half `equal`, but 
don't use a temporary variable:

  return __x.size() == __y.size() && _VSTD::equal(__x.begin(), __x.end(), 
__y.begin());

For `array` we check that sizes match and then use dual-range `equal`:

  if (_Size1 != _Size2)
  return false;
  return _VSTD::equal(__x.begin(), __x.end(), __y.begin(), __y.end());

Is there a subtle reason for this inconsistency that I'm not seeing?


https://reviews.llvm.org/D43773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40259: [libcxx] LWG2993: reference_wrapper conversion from T&

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: include/__functional_base:396
+!is_same<__uncvref_t<_Up>, reference_wrapper>::value
+>::type, bool _IsNothrow = noexcept(__bind(_VSTD::declval<_Up>()))>
+_LIBCPP_INLINE_VISIBILITY reference_wrapper(_Up&& __u) 
_NOEXCEPT_(_IsNothrow)

EricWF wrote:
> tcanens wrote:
> > Is it safe to do this when we are using `_NOEXCEPT_` in the next line?
> It should be. The noexcept condition should only be evaluated *as needed* for 
> functions selected by overload resolution. i.e. The noexcept condition is 
> only considered on well-formed functions. And this function is only 
> well-formed if `_IsNothrow` is well formed.
> 
> If `IsNothrow` is ill-formed, it will prevent the functions noexcept 
> specifier from ever being evaluated.
My point is that the use of macro-ized `_NOEXCEPT_` suggests that we are 
supporting compilers that doesn't have noexcept-specifications. Given that, can 
we then use the `noexcept` operator here?


https://reviews.llvm.org/D40259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40259: [libcxx] LWG2993: reference_wrapper conversion from T&

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: include/__functional_base:396
+!is_same<__uncvref_t<_Up>, reference_wrapper>::value
+>::type, bool _IsNothrow = noexcept(__bind(_VSTD::declval<_Up>()))>
+_LIBCPP_INLINE_VISIBILITY reference_wrapper(_Up&& __u) 
_NOEXCEPT_(_IsNothrow)

Is it safe to do this when we are using `_NOEXCEPT_` in the next line?


https://reviews.llvm.org/D40259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40144: Implement `std::launder`

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: include/new:260
+static_assert (!is_function<_Tp>::value, "can't launder functions" );
+static_assert (!is_same_v>, "can't launder cv-void" 
);
+#ifdef _LIBCPP_COMPILER_HAS_BUILTIN_LAUNDER

Technically, the attempt is to launder //pointers to// //cv// `void`/functions. 
:)

Also, since `__launder` is non-C++17-specific, I guess it can't use `_t` and 
`_v` after all... - and I suppose it'll also need to parens-protect the 
`is_same` check for the fallback `static_assert` macro?


https://reviews.llvm.org/D40144



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40259: [libcxx] LWG2993: reference_wrapper conversion from T&

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: include/__functional_base:377
+inline _LIBCPP_INLINE_VISIBILITY
+_Tp& __lvref_bind(_Tp& r) _NOEXCEPT { return r; }
+

I'd make these member functions of a class template, to avoid having to reason 
about partial ordering in overload resolution.


https://reviews.llvm.org/D40259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40144: Implement `std::launder`

[Tim Song via Phabricator via cfe-commits]

tcanens added a comment.

At least for GCC, it should use `__builtin_launder`.

Also needs to implement "The program is ill-formed if `T` is a function type or 
//cv// `void`."


https://reviews.llvm.org/D40144



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38075: Fix PR34668 - P0704R1 implementation is too permissive

[Tim Song via Phabricator via cfe-commits]

tcanens updated this revision to Diff 116490.
tcanens added a comment.

Also tweaked preceding comment.


https://reviews.llvm.org/D38075

Files:
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp


Index: test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
===
--- test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
+++ test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
@@ -3,12 +3,15 @@
 struct X {
   void ref() & {}
   void cref() const& {}
+  void cvref() const volatile& {}
 };
 
 void test() {
   X{}.ref(); // expected-error{{cannot initialize object parameter of type 'X' 
with an expression of type 'X'}}
   X{}.cref(); // expected-no-error
+  X{}.cvref(); // expected-error{{cannot initialize object parameter of type 
'const volatile X' with an expression of type 'X'}}
 
   (X{}.*::ref)(); // expected-error-re{{pointer-to-member function type 
'void (X::*)() {{.*}}&' can only be called on an lvalue}}
   (X{}.*::cref)(); // expected-no-error
+  (X{}.*::cvref)(); // expected-error-re{{pointer-to-member function type 
'void (X::*)() {{.*}}&' can only be called on an lvalue}}
 }
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -5182,8 +5182,9 @@
 
 case RQ_LValue:
   if (!isIndirect && !LHS.get()->Classify(Context).isLValue()) {
-// C++2a allows functions with ref-qualifier & if they are also 
'const'.
-if (Proto->isConst())
+// C++2a allows functions with ref-qualifier & if their 
cv-qualifier-seq
+// is (exactly) 'const'.
+if (Proto->isConst() && !Proto->isVolatile())
   Diag(Loc, getLangOpts().CPlusPlus2a
 ? 
diag::warn_cxx17_compat_pointer_to_const_ref_member_on_rvalue
 : diag::ext_pointer_to_const_ref_member_on_rvalue);


Index: test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
===
--- test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
+++ test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
@@ -3,12 +3,15 @@
 struct X {
   void ref() & {}
   void cref() const& {}
+  void cvref() const volatile& {}
 };
 
 void test() {
   X{}.ref(); // expected-error{{cannot initialize object parameter of type 'X' with an expression of type 'X'}}
   X{}.cref(); // expected-no-error
+  X{}.cvref(); // expected-error{{cannot initialize object parameter of type 'const volatile X' with an expression of type 'X'}}
 
   (X{}.*::ref)(); // expected-error-re{{pointer-to-member function type 'void (X::*)() {{.*}}&' can only be called on an lvalue}}
   (X{}.*::cref)(); // expected-no-error
+  (X{}.*::cvref)(); // expected-error-re{{pointer-to-member function type 'void (X::*)() {{.*}}&' can only be called on an lvalue}}
 }
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -5182,8 +5182,9 @@
 
 case RQ_LValue:
   if (!isIndirect && !LHS.get()->Classify(Context).isLValue()) {
-// C++2a allows functions with ref-qualifier & if they are also 'const'.
-if (Proto->isConst())
+// C++2a allows functions with ref-qualifier & if their cv-qualifier-seq
+// is (exactly) 'const'.
+if (Proto->isConst() && !Proto->isVolatile())
   Diag(Loc, getLangOpts().CPlusPlus2a
 ? diag::warn_cxx17_compat_pointer_to_const_ref_member_on_rvalue
 : diag::ext_pointer_to_const_ref_member_on_rvalue);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38075: Fix PR34668 - P0704R1 implementation is too permissive

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:5185
   if (!isIndirect && !LHS.get()->Classify(Context).isLValue()) {
 // C++2a allows functions with ref-qualifier & if they are also 
'const'.
+if (Proto->isConst() && !Proto->isVolatile())

Rakete wrote:
> I think you should update the comment to something like "also 'const', but 
> not if they're 'volatile'."
"if their cv-qualifier-seq is (exactly) 'const'", maybe?


https://reviews.llvm.org/D38075



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38075: Fix PR34668 - P0704R1 implementation is too permissive

[Tim Song via Phabricator via cfe-commits]

tcanens created this revision.
tcanens added a project: clang.

https://bugs.llvm.org/show_bug.cgi?id=34668

Pretty straightforward.


Repository:
  rL LLVM

https://reviews.llvm.org/D38075

Files:
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp


Index: test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
===
--- test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
+++ test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
@@ -3,12 +3,15 @@
 struct X {
   void ref() & {}
   void cref() const& {}
+  void cvref() const volatile& {}
 };
 
 void test() {
   X{}.ref(); // expected-error{{cannot initialize object parameter of type 'X' 
with an expression of type 'X'}}
   X{}.cref(); // expected-no-error
+  X{}.cvref(); // expected-error{{cannot initialize object parameter of type 
'const volatile X' with an expression of type 'X'}}
 
   (X{}.*::ref)(); // expected-error-re{{pointer-to-member function type 
'void (X::*)() {{.*}}&' can only be called on an lvalue}}
   (X{}.*::cref)(); // expected-no-error
+  (X{}.*::cvref)(); // expected-error-re{{pointer-to-member function type 
'void (X::*)() {{.*}}&' can only be called on an lvalue}}
 }
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -5183,7 +5183,7 @@
 case RQ_LValue:
   if (!isIndirect && !LHS.get()->Classify(Context).isLValue()) {
 // C++2a allows functions with ref-qualifier & if they are also 
'const'.
-if (Proto->isConst())
+if (Proto->isConst() && !Proto->isVolatile())
   Diag(Loc, getLangOpts().CPlusPlus2a
 ? 
diag::warn_cxx17_compat_pointer_to_const_ref_member_on_rvalue
 : diag::ext_pointer_to_const_ref_member_on_rvalue);


Index: test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
===
--- test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
+++ test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
@@ -3,12 +3,15 @@
 struct X {
   void ref() & {}
   void cref() const& {}
+  void cvref() const volatile& {}
 };
 
 void test() {
   X{}.ref(); // expected-error{{cannot initialize object parameter of type 'X' with an expression of type 'X'}}
   X{}.cref(); // expected-no-error
+  X{}.cvref(); // expected-error{{cannot initialize object parameter of type 'const volatile X' with an expression of type 'X'}}
 
   (X{}.*::ref)(); // expected-error-re{{pointer-to-member function type 'void (X::*)() {{.*}}&' can only be called on an lvalue}}
   (X{}.*::cref)(); // expected-no-error
+  (X{}.*::cvref)(); // expected-error-re{{pointer-to-member function type 'void (X::*)() {{.*}}&' can only be called on an lvalue}}
 }
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -5183,7 +5183,7 @@
 case RQ_LValue:
   if (!isIndirect && !LHS.get()->Classify(Context).isLValue()) {
 // C++2a allows functions with ref-qualifier & if they are also 'const'.
-if (Proto->isConst())
+if (Proto->isConst() && !Proto->isVolatile())
   Diag(Loc, getLangOpts().CPlusPlus2a
 ? diag::warn_cxx17_compat_pointer_to_const_ref_member_on_rvalue
 : diag::ext_pointer_to_const_ref_member_on_rvalue);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27068: Improve string::find

[Tim Song via Phabricator via cfe-commits]

tcanens added inline comments.



Comment at: include/__string:543
+_LIBCPP_CONSTEXPR_AFTER_CXX11
+_RandomAccessIterator
+__search_substring(_RandomAccessIterator __first1, _RandomAccessIterator 
__last1,

A character traits class need only accept pointers, so the name 
`_RandomAccessIterator` is misleading when you are passing them directly to 
`_Traits::find`/`_Traits::compare`. Why not just `const _CharT*`? Then you can 
strip out all the `iterator_traits` circumlocution as well.



Comment at: include/__string:548
+using __iterator_traits = iterator_traits<_RandomAccessIterator>;
+typedef typename __iterator_traits::difference_type __difference_type;
+

For example, since `_RandomAccessIterator` must be a pointer type, this can't 
possibly be anything other than `ptrdiff_t`.



Comment at: include/__string:568
+  __first1 = _Traits::find(__first1, __len1 - __len2 + 1, __f2);
+  if (__first1 == _RandomAccessIterator(0))
+return __last1;

The function-style cast is redundant.


https://reviews.llvm.org/D27068



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits