[PATCH] D60934: [clang] adding explicit(bool) from c++2a

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete1111 added a comment.

Thanks for working on this! :)



================
Comment at: clang/lib/Parse/ParseDecl.cpp:3533
+          if (ExplicitExpr.isInvalid()) {
+            Diag(ParenLoc, diag::note_explicit_bool_breaking_change_cxx2a)
+                << FixItHint::CreateReplacement(
----------------
This is a useful diagnostic but you'll need to fix (a lot) of false positives:

```
template <typename T> struct Foo { explicit(T{} +) Foo(); };
```

gets me:

```
main.cpp:1:50: error: expected expression
template <typename T> struct Foo { explicit(T{} +) Foo(); };
                                                 ^
main.cpp:1:44: note: this expression is parsed as explicit(bool) since C++2a
template <typename T> struct Foo { explicit(T{} +) Foo(); };
                                   ~~~~~~~~^
                                   explicit(true)
```

Fixit hints should only be used when it is 100% the right fix that works, always.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3534
+            Diag(ParenLoc, diag::note_explicit_bool_breaking_change_cxx2a)
+                << FixItHint::CreateReplacement(
+                       SourceRange(ExplicitLoc, ExplicitLoc.getLocWithOffset(
----------------
FixIt Hints also need tests :)


================
Comment at: clang/lib/Sema/DeclSpec.cpp:952
+  assert((ExplicitState != ESF_unresolved || ExplicitExpr) &&
+         "ExplicitExpr can't be null if the state is ESF_resolved_false or "
+         "ESF_unresolved");
----------------
The comment seems to explain something else than the code.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:956
   // intended.
-  if (FS_explicit_specified) {
+  // TODO : it is unclear how to handle multiple explicit(bool) specifiers.
+  if (hasExplicitSpecifier()) {
----------------
We accept `explicit explicit`, but we really shouldn't accept `explicit(some-expr) explicit(some-other-expr)`. That would just be confusing. `explicit explicit(some-expr)` is also borderline.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1316
     FixItHint Hint = FixItHint::CreateRemoval(SCLoc);
     S.Diag(SCLoc, diag::err_friend_decl_spec)
       << Keyword << Hint;
----------------
Please change this warning so that the whole *explicit-specifier* is underlined (or even change the text for conditional explicit):

```
main.cpp:1:36: error: 'explicit' can only be applied to a constructor or conversion function
template <typename T> struct Foo { explicit(false) void foo(); };
                                   ^~~~~~~~
1 error generated.
```

This will also fix the FixIt Hint.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8641
   // C++0x explicit conversion operators.
-  if (DS.isExplicitSpecified())
+  if (DS.hasExplicitSpecifier())
     Diag(DS.getExplicitSpecLoc(),
----------------
You should amend the diagnostic, because as of right now `explicit(true)` is being diagnosed as C++11 feature.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10870
+  ExprResult Converted =
+      CheckBooleanCondition(Loc, ExplicitExpr, /*isConstexpr=*/true);
+  if (Converted.isInvalid())
----------------
This assumes that we are in a [constexpr] if, leading to errors like this:

```
int a;
template <typename T> struct Foo { explicit(a) Foo(); };
```

```
main.cpp:2:45: error: constexpr if condition is not a constant expression
template <typename T> struct Foo { explicit(a) Foo(); };
                                            ^
```


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10876-10881
+    return Converted;
+  }
+
+  llvm::APSInt Result;
+  Converted = VerifyIntegerConstantExpression(
+      Converted.get(), &Result, diag::err_noexcept_needs_constant_expression,
----------------
Wrong diagnostic.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:10359
+  default:
+    llvm_unreachable("invalide Decl");
+  }
----------------
typo.



================
Comment at: clang/lib/Sema/SemaOverload.cpp:10361
+  }
+  assert(ESI.getPointer() && "null expression should be handled before");
+  S.Diag(Cand->Function->getLocation(),
----------------
This fires for an instantiation dependent (but not value dependent) explicit specifier that is invalid:

```
template <typename T> struct Foo { explicit((T{}, false)) Foo(int); };

int main() { Foo<void> f = 1; }
```


================
Comment at: clang/test/SemaCXX/cxx2a-compat.cpp:46
+#if __cplusplus <= 201703L
+// expected-warning at -3 {{this expression would be parsed as explicit(bool) in C++2a}}
+#if defined(__cpp_conditional_explicit)
----------------
would -> will


================
Comment at: clang/test/SemaCXX/cxx2a-compat.cpp:51
+#else
+// expected-error at -8 {{does not refer to a value}}
+// expected-error at -9 {{expected member name or ';'}}
----------------
A fixit hint for this would be great. Also it would be nice if there was a nicer error message.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60934/new/

https://reviews.llvm.org/D60934