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

[Tyker via Phabricator via cfe-commits]

Tyker marked an inline comment as done.
Tyker added a comment.

Fixed based on Feedback from @rsmith @martong @Rakete1111.

feedback that weren't fixed have comment explaining why.



================
Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+  void setExplicitSpecifier(ExplicitSpecInfo ESI);
+
----------------
Tyker wrote:
> rsmith wrote:
> > Generally we don't want to have setters in the AST; the AST is intended to be immutable after creation. Is this necessary?
> this is used in 2 cases:
> - for deserialization.
> - for in `Sema::ActOnFunctionDeclarator` to make every declaration of the same function have the same explicit specifier as the canonical one. there is probably a better way to do this but i didn't find how.
> 
> the second use case will need to be changed because the constructor's explicit specifier will be tail-allocated so the explicit specifier from the canonical declaration will need to be recovered before construction to allocate storage. 
> 
> how can we find the canonical declaration of a function before it is constructed ?
i found a solution around that issue.
now setExplicitSpecifier is only used for deserialization.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:40
+def note_explicit_bool_breaking_change_cxx2a : Note<
+  "this expression is parsed as explicit(bool) since C++2a">;
+
----------------
rsmith wrote:
> This should be a warning in the `CXXPre2aCompat` group, phrased as "explicit(bool) is incompatible with C++ standards before C++2a".
explicit(bool) can only be parse with the c++2a option because code like:
```
 struct C {
  explicit(C)(int);
 };
```
is correct before c++2a but is parsed as explicit(bool) and fail to complie in c++2a.
so i don't think this warning can be fired in any case. so i removed it.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3533
+          if (ExplicitExpr.isInvalid()) {
+            Diag(ParenLoc, diag::note_explicit_bool_breaking_change_cxx2a)
+                << FixItHint::CreateReplacement(
----------------
rsmith wrote:
> Rakete1111 wrote:
> > 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.
> This "add a note after an error" construct is an anti-pattern, and will fail in a bunch of cases (for example: if multiple diagnostics are produced, it gets attached to the last one rather than the first; if a disabled warning / remark is produced last, the note is not displayed at all).
> 
> As noted above, the conventional way to handle this is to unconditionally produce a `-Wc++17-compat` warning when we see `explicit(`. Attaching a context note to the diagnostic if building the expression fails (catching both `Parse` and `Sema` diagnostics) will require more invasive changes and I'd prefer to see that left to a separate patch (if we do it at all).
after the comment from rsmith i will remove it at least for now.


================
Comment at: clang/lib/Sema/SemaInit.cpp:9361
         // Only consider converting constructors.
-        if (GD->isExplicit())
+        if (!GD->isMaybeNotExplicit())
           continue;
----------------
rsmith wrote:
> We need to substitute into the deduction guide first to detect whether it forms a "converting constructor", and that will need to be done inside `AddTemplateOverloadCandidate`.
similarly as the previous if. this check removes deduction guide that are already resolve to be explicit when we are in a context that doesn't allow explicit.
every time the explicitness was checked before my change i replaced it by a check that removes already resolved explicit specifiers.


================
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 ';'}}
----------------
Rakete1111 wrote:
> A fixit hint for this would be great. Also it would be nice if there was a nicer error message.
this had a fix it in the previous patch with the "this expression is parsed as explicit(bool) since C++2a" note. but i removed it because to much false positive and fixing it would make the patch even bigger.


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

https://reviews.llvm.org/D60934