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@-8 {{does not refer to a value}}
+// expected-error@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
