This is an archive of the discontinued LLVM Phabricator instance.

[clang] adding explicit(bool) from c++2a
ClosedPublic

Authored by Tyker on Apr 20 2019, 4:22 AM.

Details

Reviewers
rsmith
shafik
martong
Commits
rZORG4d042e32f390: Simplify tracking of end of consumed decl-specifier sequence.
rZORG7dce45d93279: [c++20] Add support for explicit(bool), as described in P0892R2.
rZORGd336f6eef04d: Fix up lldb after clang r360311.
rZORGe0b03a5b769b: [clang] adding explicit(bool) from c++2a
rZORG6440f3ea80ce: Simplify tracking of end of consumed decl-specifier sequence.
rZORG3fd33c7236a4: Fix up lldb after clang r360311.
rZORG9fdaf634e692: [c++20] Add support for explicit(bool), as described in P0892R2.
rZORG0ec843eafe61: [clang] adding explicit(bool) from c++2a
rG4d042e32f390: Simplify tracking of end of consumed decl-specifier sequence.
rGd336f6eef04d: Fix up lldb after clang r360311.
rG7dce45d93279: [c++20] Add support for explicit(bool), as described in P0892R2.
rGe0b03a5b769b: [clang] adding explicit(bool) from c++2a
rG6440f3ea80ce: Simplify tracking of end of consumed decl-specifier sequence.
rG3fd33c7236a4: Fix up lldb after clang r360311.
rG9fdaf634e692: [c++20] Add support for explicit(bool), as described in P0892R2.
rG0ec843eafe61: [clang] adding explicit(bool) from c++2a
rGba24f352f4fa: Simplify tracking of end of consumed decl-specifier sequence.
rL360369: Simplify tracking of end of consumed decl-specifier sequence.
rC360369: Simplify tracking of end of consumed decl-specifier sequence.
rG36851a66c8c9: Fix up lldb after clang r360311.
rL360312: Fix up lldb after clang r360311.
rLLDB360312: Fix up lldb after clang r360311.
rG76b9027f352a: [c++20] Add support for explicit(bool), as described in P0892R2.
rC360311: [c++20] Add support for explicit(bool), as described in P0892R2.
rL360311: [c++20] Add support for explicit(bool), as described in P0892R2.
rG5fe2ddbdf47d: [clang] adding explicit(bool) from c++2a
rL359949: [clang] adding explicit(bool) from c++2a
rC359949: [clang] adding explicit(bool) from c++2a
Summary

this patch adds support for the explicit bool specifier.

Changes:

  • The parsing for the explicit(bool) specifier was added in ParseDecl.cpp.
  • The explicit specifier is now stored everywhere with the ExplicitSpecifier class.
  • Following the AST change, Serialization, ASTMatchers, ASTComparator and ASTPrinter were adapted.
  • Template instantiation was adapted to instantiate the potential expressions of the explicit(bool) specifier When instantiating their associated declaration.
  • The Add*Candidate functions were adapted, they now take a Boolean indicating if the context allowing explicit constructor or conversion function and this boolean is used to remove invalid overloads that required template instantiation to be detected.
  • Test for Semantic and Serialization were added.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
Tyker edited the summary of this revision. (Show Details)Apr 20 2019, 4:23 AM
Quuxplusone added inline comments.
clang/include/clang/AST/DeclCXX.h
2694

s/Specifer/Specifier/ (and please git grep for other instances of the same typo)

clang/include/clang/Basic/DiagnosticParseKinds.td
40

"C++2a" should be uppercased.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2120

s/specfier/specifier/ (and please git grep for other instances)

clang/lib/Sema/DeclSpec.cpp
957

Spelling/grammar/capitalization-of-C++2a.
Also, it seems to me that you've got a CWG wording issue here: what does N4810 mean by "Each decl-specifier shall appear at most once in a complete decl-specifier-seq, except that long may appear twice"? What is "each" decl-specifier? Is explicit(true) a different decl-specifier from explicit(1+1==2)? Is explicit(true) different from explicit(false)?

clang/test/SemaCXX/explicit.cpp
189

Why?

This patch certainly took a few hours to write. Can you spend 5 minutes on making the summary readable (spelling, capitalization, formatting, well-formed sentences, ...) ?

Tyker edited the summary of this revision. (Show Details)Apr 20 2019, 9:11 AM
Tyker edited the summary of this revision. (Show Details)
Tyker edited the summary of this revision. (Show Details)Apr 20 2019, 9:13 AM
Tyker marked 2 inline comments as done.Apr 20 2019, 9:19 AM
Tyker added inline comments.
clang/lib/Sema/DeclSpec.cpp
957

I agree that it isn't clear. but i assumed that each counts every explicit-specifier as one.

clang/test/SemaCXX/explicit.cpp
189

i added a run line with the -std=c++2a flag to ensure no regression in c++2a mode as well as in c++11 mode. the warning that is being tested doesn't appear with this flag. the #if is to prevent the test from failling with the -stdc++2a flag. i don't know if the flag's behavior is intended or not.
godbolt example

Tyker updated this revision to Diff 195971.Apr 20 2019, 9:47 AM
Tyker updated this revision to Diff 195973.Apr 20 2019, 9:51 AM
Tyker edited the summary of this revision. (Show Details)Apr 20 2019, 12:20 PM
Tyker edited the summary of this revision. (Show Details)Apr 20 2019, 2:18 PM
Tyker updated this revision to Diff 195990.Apr 21 2019, 1:58 AM

@Quuxplusone @riccibruno fixed / answered feedback

Tyker marked 3 inline comments as done.Apr 21 2019, 1:59 AM
martong added inline comments.Apr 23 2019, 7:23 AM
clang/lib/AST/ASTImporter.cpp
3131

Why is it needed to import the explicit specifier here again?
You already imported that above. Perhaps this hunk is not needed here at all?

clang/lib/AST/ASTStructuralEquivalence.cpp
965–966

Would it work if Method1 has a different ASTContext than `Method2?
That is exactly the case when we import an AST into another with ASTImporter.

Tyker updated this revision to Diff 196285.Apr 23 2019, 11:00 AM

Fixed issues form feedback by @martong

Added:

  • CTAD and deduction guide support.
  • improved tests
Tyker marked 2 inline comments as done.Apr 23 2019, 11:01 AM

Thanks for working on this! :)

clang/lib/Parse/ParseDecl.cpp
3537

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.

3538

FixIt Hints also need tests :)

clang/lib/Sema/DeclSpec.cpp
952

The comment seems to explain something else than the code.

954–955

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.

1318

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.

clang/lib/Sema/SemaDeclCXX.cpp
8637

You should amend the diagnostic, because as of right now explicit(true) is being diagnosed as C++11 feature.

10869

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(); };
                                            ^
10875–10880

Wrong diagnostic.

clang/lib/Sema/SemaOverload.cpp
10349

typo.

10351

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; }
clang/test/SemaCXX/cxx2a-compat.cpp
46

would -> will

51

A fixit hint for this would be great. Also it would be nice if there was a nicer error message.

Fixed issues form feedback by @martong

Thank you for addressing the comments. ASTImporter.cpp and ASTStructuralEquivalence.cpp looks good to me now. However, about the equivalence check, do you think it would be possible to have a unit test for explicit(bool) in StructuralEquivalenceTest.cpp? That test would exercise the static EquivalentExplicit() function too.

rsmith marked an inline comment as done.Apr 25 2019, 11:51 AM
rsmith added inline comments.
clang/include/clang/AST/DeclCXX.h
2071

I think this is too special-case to live on CXXDeductionGuideDecl. Instead, I suggest:

  • Add a class wrapping ExplicitSpecInfo, maybe called ExplicitSpecifier
  • Change all the AST interfaces that deal in ExplicitSpecInfo to instead deal in ExplicitSpecifier objects
  • Move this comparison logic there, along with the more uncommon members you add below.

I think we should be able to reduce the set of explicit-related members on the various Decl subclasses to just getExplicitSpecifier() and isExplicit().

2082

Generally we don't want to have setters in the AST; the AST is intended to be immutable after creation. Is this necessary?

2552

Consider storing this in a TrailingObject rather than making all constructors a pointer wider for this (presumably relatively uncommon) special case.

clang/include/clang/Basic/DiagnosticParseKinds.td
37

would be -> will be

38

Nit: comma on previous line, please.

40

This should be a warning in the CXXPre2aCompat group, phrased as "explicit(bool) is incompatible with C++ standards before C++2a".

clang/include/clang/Basic/DiagnosticSemaKinds.td
2119–2124

I don't believe there is any such rule; instead, the rule is simply that deduction guides cannot be redeclared at all. See [temp.deduct.guide]/3: "Two deduction guide declarations in the same translation unit for the same class template shall not have equivalent parameter-declaration-clauses." (That's obviously wrong; we should compare the template-head as well, but the intention is that deduction guides cannot be redeclared.)

clang/include/clang/Basic/Specifiers.h
27–29

Please capitalize these enumerator names, and consider using an 'enum class' instead of an ESF_ prefix. (We use lowercase for the next few enums because their enumerators are (prefixed) keywords.)

clang/include/clang/Sema/DeclSpec.h
377

If this is a fully-semantically-analyzed explicit-specifier, this should use the same type we use in the AST representation. If it's a parsed-but-not-yet-semantically-analyzed explicit-specifier, using ExplicitSpecFlag doesn't seem right: you haven't tried to resolve it yet in that case.

clang/lib/AST/DeclCXX.cpp
1867–1878

Please don't evaluate the explicit specifier here. Instead, it should be evaluated by Sema when it's initially formed (if it's not value-dependent) and during template instantiation (if a value-dependent explicit-specifier becomes non-value-dependent). Note that Sema needs to evaluate it at those times anyway, in order to properly diagnose non-constant explicit-specifiers.

2467–2475

I don't think this should work this way any more: we can't tell whether a constructor template forms a converting constructor before substituting into it.

Another point of note: the "that can be called with a single parameter" part of the language rule has been removed (and was never necessary). So "isConvertingConstructor" is really just "is not explicit".

So: I think we should rename this function to a more general "could this function ever be called with exactly N arguments?" function (moved to FunctionDecl, adjacent to getMinRequiredArguments), and use it in the current callers of isConvertingConstructor just to filter out functions that can't be called with exactly one argument from the candidate list for copy-initialization. Then change the current callers of this function to call Add*OverloadCandidate with AllowExplicit = false in the cases where they currently require a converting constructor.

It'd make sense to do that as a separate change / refactoring before adding explicit(bool) support.

clang/lib/AST/DeclPrinter.cpp
555

Function names should generally be verb phrases, not noun phrases. printExceptionSpecifier maybe?

clang/lib/Parse/ParseDecl.cpp
3527

Please try to avoid using lookahead for this. (You may need to change the code after the loop so you can tell it that you already consumed the specifier, or factor out that code into a lambda that you can call from both there and here.)

3537–3541

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).

clang/lib/Sema/DeclSpec.cpp
957

I reported this on the core reflector nearly a year ago (http://lists.isocpp.org/core/2018/06/4673.php); sadly the issue process is a long way behind so we don't have an issue number yet, but I think the intent is clear, and matches what is implemented here.

clang/lib/Sema/SemaDeclCXX.cpp
10865

Return an ExplicitSpecifier here rather than splitting it up into an ExprResult and an ExplicitSpecFlag.

10869

This is wrong; a "condition" is the thing that appears after if ( or while ( or in the middle term of a for (;;.

An explicit-specifier requires a contextually-converted constant expression of type bool, so you should call CheckConvertedContantExpression, and add a new CCEKind for this conversion (look at what we do for CCEK_ConstexprIf to find how to get it to convert to bool). You'll need to update its CCEKind-specific diagnostics (eg, ext_cce_narrowing, err_expr_not_cce) to properly diagnose explicit-specifiers.

clang/lib/Sema/SemaInit.cpp
3817

Please use a variable with a different name here, such as AllowExplicitConversion. Changing a variable with a larger scope to match the semantics required in a smaller scope like this is error-prone (even though the code is correct right now). Also, please hoist that new variable out of the for loop.

3818

Consider just removing this if. It's not clear that it carries its weight.

5000–5001

Likewise.

9361

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.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
387–403

Please factor out the common code between this and ActOnExplicitSpecifier. (We usually call such a function BuildSomething, such as Sema::BuildExplicitSpecifier).

388

This needs to be done in a ConstantEvaluated context too.

martong added inline comments.Apr 26 2019, 8:07 AM
clang/lib/AST/ASTStructuralEquivalence.cpp
965–966

isEquivalentExplicit calls EquivalentExplicit, which in turn calls Stmt::Profile.
Seems like Stmt::Profile relies on pointer equivalencies, however, here we may have two different ASTContexts for each Method, so we must not rely on pointer values.
I suggest to use a version of EquivalentExplicit which uses Stmt::ProcessODRHash, that one does not rely on pointer values.

Tyker marked 2 inline comments as done.Apr 26 2019, 9:35 AM
Tyker added inline comments.
clang/include/clang/AST/DeclCXX.h
2082

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 ?

clang/lib/Sema/SemaInit.cpp
3818

this if prevent conversion that are already known not to be valid from being added to the overload set. removing it is still correct because it will be removed later. but we would be doing "useless" work because we are adding the to the overload set knowing they will be removed.
the only benefit i see of removing this if is to make all explicit conversion appear in overload resolution failure messages. is it the intent ?

Tyker updated this revision to Diff 197043.EditedApr 28 2019, 3:52 PM
Tyker marked 34 inline comments as done.
Tyker marked an inline comment as done.

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

feedback that weren't fixed have comment explaining why.

notable changes:

  • ExplicitSpecInfo that was a type alias on llvm::PointerIntPair<Expr*, 2, ExplicitSpecFlag> is now replaced everywhere by the ExplicitSpecifier class.
  • CXXConstructorDecl now stores its ExplicitSpecfier in the TraillingObjects.
  • most explicit related function in CXXConstructorDecl, CXXConversionDecl and CXXDeductionGuideDecl have been moved to ExplicitSpecifier.
  • removed the note "this expression is parsed as explicit(bool) since C++2a" as it had to many false positive and fixing them would be quite complicated.
  • fixed an issue where ExplicitSpecifiers loaded from a PCH weren't exactly similar to the original.
  • fixed issues with Diagnostics underlining the explicit keyword instead of the whole explicit-specifier.
  • having multiple explicit-specifier with at least one having an expression is now an error instead of a warning.
  • redeclaring a deduction guide for which the arguments are equivalent the previous one is now always an error (as specified by the standard). instead of emitting an error only when explicit-specifiers don't match. test were adapted accordingly.
  • ExplicitSpecifier::isEquivalent the replacement for EquivalentExplicit is now based on ODRHashs.
  • ExplicitSpecifiers are now evaluated using CheckConvertedConstantExpression.
  • Added tests in StructuralEquivalenceTest.cpp for the ExplicitSpecifier
clang/include/clang/AST/DeclCXX.h
2082

i found a solution around that issue.
now setExplicitSpecifier is only used for deserialization.

clang/include/clang/Basic/DiagnosticParseKinds.td
40

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.

clang/lib/Parse/ParseDecl.cpp
3537

after the comment from rsmith i will remove it at least for now.

clang/lib/Sema/SemaInit.cpp
9361

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.

clang/test/SemaCXX/cxx2a-compat.cpp
51

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.

Tyker updated this revision to Diff 197085.Apr 29 2019, 4:21 AM

merged similar code for AST serialization of ExplicitSpecifier in a static function

martong accepted this revision.Apr 29 2019, 4:43 AM

ASTImporter.cpp and ASTStructuralEquivalence.cpp looks good to me now. I am resigning as a reviewer since I don't feel competent enough to review the rest of the change.

This revision is now accepted and ready to land.Apr 29 2019, 4:43 AM
martong resigned from this revision.Apr 29 2019, 4:44 AM
This revision now requires review to proceed.Apr 29 2019, 4:44 AM
rsmith marked 2 inline comments as done.Apr 29 2019, 5:22 PM

Thanks, this is looking really good. (Lots of comments but they're mostly mechanical at this point.)

clang/include/clang/AST/DeclBase.h
1539–1546

I would prefer that we keep an explicit number here so that we can ensure that this field has the range we desire.

1542–1546

Typo "Wether" (x2).

1543–1546

Maybe HasSimpleExplicit, HasTrailingExplicitSpecifier would be better names? (Please capitalize these to match the surrounding style.)

clang/include/clang/AST/DeclCXX.h
1995

Usually we'd call this Kind, not Flag.

rsmith added inline comments.Apr 29 2019, 5:22 PM
clang/include/clang/AST/DeclCXX.h
2013

"this" -> "This".

2018–2020

Do we need this? = ExplicitSpecifier(E, Kind) seems just as good and arguably clearer.

clang/include/clang/ASTMatchers/ASTMatchers.h
6177

This will match explicit(false) constructors. I think getExplicitSpecifier().isExplicit() would be better, but please also add a FIXME here: it's not clear whether this should match a dependent explicit(....).

clang/include/clang/Basic/DiagnosticSemaKinds.td
2119

Drop the "a" here.

clang/include/clang/Basic/Specifiers.h
26–28

My apologies, I led you the wrong way: the style to use for these is ResolvedFalse, ResolvedTrue, Unresolved. (I meant to capitalize only the first letter, not the whole thing, in my prior comment.)

31

Instead of exposing this serialization detail here, please localize this to the serialization code. One nice way is to serialize (Kind << 1) | HasExpr (use the bottom bit for the flag, not the top bit).

clang/include/clang/Sema/DeclSpec.h
437

Default-construct this.

572–574

Can you use FS_explicit_specifier.isSpecified() here?

591–593

Can you use = ExplicitSpecifier();?

clang/include/clang/Sema/Sema.h
10124

Missing last word from this comment?

clang/include/clang/Serialization/ASTBitCodes.h
1441–1446

For previous similar cases we've put the necessary data to pass into ::CreateDeserialized(...) at the start of the record (eg, see the Record.readInt() calls in ReadDeclRecord). We're starting to get a combinatorial explosion here (all 4 combinations of the two possible trailing objects), so maybe now's the time for that.

clang/include/clang/Serialization/ASTReader.h
2434

For consistency with nearby code, please name this variable starting with a capital letter.

clang/lib/AST/DeclCXX.cpp
1905

Default-construct.

2368–2369

Default-construct.

2370

This leaves the tail-allocated ExplicitSpecifier uninitialized, because the constructor thought there was no explicit specifier; consider calling setExplicitSpecifier(ExplicitSpecifier()); here if hasTailExplicit.

2543

Default-construct.

clang/lib/Parse/ParseDecl.cpp
3535

There's no guarantee that you have an r_paren here. Use Tracker.consumeClose() instead; it will do the right thing (including producing a suitable error if the next token is something other than a )).

3539–3542

Now we have a representation for an invalid *explicit-specifier*, I think you should be able to just carry on here and parse more specifiers and the rest of the declaration.

3904–3918

In the isAlreadyConsumed case, Tok will be the wrong token here (it'll be the token after the explicit-specifier). Factoring this out into a lambda taking a SourceLocation (to be called from the explicit case) would help; alternatively you could grab the SourceLocation of Tok before the switch and use it here instead of Tok.

clang/lib/Sema/DeclSpec.cpp
958

The latter case should be ext_ not warn_: under CWG issue 1830 (http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1830) repeated decl-specifiers (including explicit) are ill-formed (and that issue is in DR status, so should be applied retroactively).

1323

Default-construct.

clang/lib/Sema/SemaDeclCXX.cpp
10880

Loc here is unused; can you remove it?

11037

Default-construct.

12609–12611

Default-construct.

12740–12742

Default-construct.

clang/lib/Sema/SemaInit.cpp
3817–3831

We no longer pass false for AllowExplicit to Add*Candidate when CopyInitializing is true. Is that an intentional change?

3818

The intent was to simplify the logic here, and to have only one place where we check the same explicit-specifier (rather than checking it twice, with one check sometimes being incomplete). Let's leave this alone for now, to keep this patch smaller.

9361

Unlike in the previous case, we do not yet pass an AllowExplicit flag into AddTemplateOverloadCandidate here, so this will incorrectly allow dependent explicit-specifiers that evaluate to true in copy-initialization contexts.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
375

Please add an ExplicitSpecifier ExplicitSpecifier::invalid() static member function to build the "invalid" state, rather than hardcoding the invalid state here.

384

return ExplicitSpecifier::invalid(); here, and don't declare Result until you are ready to initialize it below.

1760–1763

C++ language rules require that we substitute into the declaration in lexical order (this matters for SFINAE versus non-immediate-context errors). You should instantiate the explicit specifier prior to the call to SubstFunctionType to match source order.

2076–2080

Likewise, this should be done after instantiating the template parameter lists and before substitution into the function type.

clang/lib/Serialization/ASTReaderDecl.cpp
2021

Please delete this.

clang/test/CXX/temp/temp.deduct.guide/p3.cpp
8–9

Please delete this (out of date) comment.

Tyker marked 2 inline comments as done.Apr 30 2019, 3:53 AM
Tyker added inline comments.
clang/include/clang/AST/DeclBase.h
1539–1546

couldn't we compute the value and static_assert on it. having to modify this each time we modify DeclContextBits or FunctionDeclBits is sad. and there isn't anything reminding us to do so in some cases.
what would be a reasonable minimum ?

clang/lib/Sema/SemaInit.cpp
3817–3831

yes. i didn't found any cases in which AllowExplicit was false but CopyInitializing was true. so i assumed that relying only on AllowExplicit is better. the regression tests passes after the change.

Tyker updated this revision to Diff 197378.Apr 30 2019, 10:57 AM
Tyker marked 37 inline comments as done.

@rsmith fixed feedbacks

except one i commented on.

clang/include/clang/AST/DeclCXX.h
2082

as it is only used by deserialization and ASTDeclReader is friend to all class with a setExplicitSpecifier.
I made setExplicitSpecifier private.

clang/include/clang/ASTMatchers/ASTMatchers.h
6177

shouldn't this be able to match with deduction guides too ?

clang/lib/Sema/SemaInit.cpp
9361

the default value for AllowExplicit is false. so the conversion will be removed in AddTemplateOverloadCandidate.

This is great, thank you so much!

clang/include/clang/AST/DeclBase.h
1539–1546

I think it'd be reasonable to reduce this to 20 if you like. (Going down as far as 16 or so seems acceptable, but I'd be worried about generated code having thousands of members, so I'd be hesitant to go below that.) The existing static_assert on line ~1721 will then catch if we add too many bit-field members.

clang/include/clang/AST/DeclCXX.h
2603

Please wrap this to 80 columns.

clang/include/clang/ASTMatchers/ASTMatchers.h
6174

Typo "FIXEME" -> "FIXME"

6177

Yes, it should. This patch is doing a lot of things already, though, so I'd prefer that we fix that as a separate change.

clang/lib/Parse/ParseDecl.cpp
3915

These diagnostics need to be updated to use a different location (rather than using Tok) if isAlreadyConsumed.

clang/lib/Sema/SemaInit.cpp
9361

That doesn't sound right: that'd mean we never use explicit deduction guides (we never pass AllowExplicit = true to AddTemplateOverloadCandidate). Do we have any test coverage that demonstrates that conditionally-explicit deduction guides are handled properly?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2077

Please reflow to 80 columns.

xbolva00 added inline comments.
clang/include/clang/AST/DeclBase.h
1534

Small typo + update comment

Rakete1111 added inline comments.Apr 30 2019, 3:58 PM
clang/include/clang/AST/DeclCXX.h
2579

Your or needs parens or the disambiguation is wrong.

clang/include/clang/Serialization/ASTReader.h
2435

same here.

clang/lib/Sema/DeclSpec.cpp
952

typo.

clang/test/CXX/temp/temp.deduct.guide/p1.cpp
74

Is there a reason why you changed this?

Tyker updated this revision to Diff 197525.May 1 2019, 3:44 AM
Tyker marked 15 inline comments as done.

fixed feedback from @rsmith @Rakete1111 @xbolva00

clang/include/clang/AST/DeclCXX.h
2579

i don't understand. but there is no need for disambiguation, a && true == a so this will work regardless of operator priority.

clang/include/clang/Serialization/ASTReader.h
2435

what is the issue

clang/lib/Sema/SemaInit.cpp
9361

my mistake. the default value for AllowExplicit is false. but AddTemplateOverloadCandidate will only remove conversions and constructors. dependent explicit specifier that are resolved to true on deduction guides are removed at line 9480. they are not removed from the overload set. CTAD just fails if a explicit deduction guide is selected in a CopyInitList. i agree this is weird but the behavior is the same as before the patch.
the result on clang is:

template<typename T>
struct A {
  explicit A(T);
};
A a = 0; // error with explicit constructor meaning CTAD succeed.
A a = { 0 }; // error with explicit deduction guide

all compiler don't agree on this https://godbolt.org/z/ElHlkE. icc and clang have this behavior, gcc and msvc fail at CTAD time on both initialization. as of what the standard say from what i read, it isn't clear, the standard is clear when calling an explicit constructor should fail. but it doesn't appear to say for deduction guides.
as this was the previous behavior i did not change it with explicit(bool).

clang/test/CXX/temp/temp.deduct.guide/p1.cpp
74

yes. One of change that was made is making deduction guide redeclaration be an error. Without this change, this line was a redeclartion so the test didn't serve its purpose.

Rakete1111 added inline comments.May 1 2019, 12:22 PM
clang/include/clang/AST/DeclCXX.h
2579

Yeah sorry you're right, I meant that the parens are needed to shut up -Wparens warning about disambiguation.

clang/include/clang/Serialization/ASTReader.h
2435

For consistency with nearby code, please name this variable starting with a capital letter.

rsmith added inline comments.May 1 2019, 12:58 PM
clang/lib/Sema/SemaInit.cpp
9361

the standard is clear when calling an explicit constructor should fail. but it doesn't appear to say for deduction guides

The standard says that you take the set of deduction guides and notionally form a set of constructors from them (see [over.match.class.deduct]). So the constructor rules apply to deduction guides too.

as this was the previous behavior i did not change it with explicit(bool).

I don't think that's correct. We filter out explicit deduction guides for non-list copy-initialization on line ~9239 (prior to this change). Today we get this result:

template<typename T> struct X { X(int); };

explicit X(int) -> X<int>; // #1

X a = 0; // error: no viable deduction guide, #1 is not a candidate
X b = {0}; // error: selected explicit deduction guide #1
X c{0}; // ok
X d(0); // ok

... which is correct. If we change the deduction guide to have a dependent explicit(bool) specifier:

template<bool E = true>
explicit(E) X(int) -> X<int>;

... we should get the same result, but I think we won't get that result with this patch: I think we'll incorrectly select an explicit deduction guide for the declaration of a, because we never filter out explicit deduction guides.

DeduceTemplateSpecializationFromInitializer should compute an AllowExplicit flag (= !Kind.isCopyInit() || ListInit), pass it into AddTemplateOverloadCandidate, and that should filter out explicit deduction guide specializations if it's true.

Tyker updated this revision to Diff 197722.May 2 2019, 2:18 AM
Tyker marked 9 inline comments as done.

fixed feedback from @rsmith @Rakete1111.

clang/lib/Sema/SemaInit.cpp
9361

there was an issue. now fixed.

rsmith accepted this revision.May 2 2019, 3:22 PM

Looks good, thank you!

clang/lib/Sema/SemaInit.cpp
9359

Please simplify this to if (!AllowExplicit)

This revision is now accepted and ready to land.May 2 2019, 3:22 PM
Tyker updated this revision to Diff 197929.EditedMay 3 2019, 1:58 AM

@rsmith I made the last requested few changes. But i can't push the change, i don't have the permission.

This revision was automatically updated to reflect the committed changes.
Tyker added a comment.May 6 2019, 4:29 AM

yes i am on it.

Tyker updated this revision to Diff 198289.EditedMay 6 2019, 8:57 AM
Tyker edited the summary of this revision. (Show Details)

Changes:

sorry for the inconvenience.

rsmith reopened this revision.May 6 2019, 1:16 PM
rsmith added inline comments.
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
373–374

This is incorrect: you still need to substitute into an explicit-specifier even if you've already resolved it, at least if it's instantiation-dependent. (Substitution could fail, and if it does, you need to produce the error.) For example:

template<typename T> struct A {
  explicit(sizeof(sizeof(T::error)) == sizeof(sizeof(int))) A();
};

Here, the expression in the explicit-specifier is neither type-dependent nor value-dependent, but it is instantiation-dependent, and substitution into it should fail when, say, T is int. (Instantiating A<int> should fail with an error.)

This revision is now accepted and ready to land.May 6 2019, 1:16 PM
Tyker updated this revision to Diff 198349.EditedMay 6 2019, 3:09 PM

fixed that issue

Tyker marked an inline comment as done.May 6 2019, 3:11 PM
Tyker added a comment.May 7 2019, 2:02 AM

could you commit this for me ?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 8:58 PM
RKSimon added a subscriber: RKSimon.May 9 2019, 3:58 AM
RKSimon added inline comments.
cfe/trunk/lib/Parse/ParseDecl.cpp
3939 ↗(On Diff #198758)

@Tyker @rsmith We're seeing -Wparentheses warnings because of this - please can you take a look? The logic looks a little dodgy for a both/neither assertion as well.

http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/27322/steps/build%20stage%201/logs/warnings%20%281%29

Tyker marked an inline comment as done.May 9 2019, 6:57 AM
Tyker added inline comments.
cfe/trunk/lib/Parse/ParseDecl.cpp
3939 ↗(On Diff #198758)

i made a revision that fixes this: https://reviews.llvm.org/D61731