Page MenuHomePhabricator

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

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

Details

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

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Apr 29 2019, 5:22 PM
clang/include/clang/AST/DeclCXX.h
2013 ↗(On Diff #197085)

"this" -> "This".

2018–2020 ↗(On Diff #197085)

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

clang/include/clang/ASTMatchers/ASTMatchers.h
6174 ↗(On Diff #197085)

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 ↗(On Diff #197085)

Drop the "a" here.

clang/include/clang/Basic/Specifiers.h
26–28 ↗(On Diff #197085)

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 ↗(On Diff #197085)

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 ↗(On Diff #197085)

Default-construct this.

572–574 ↗(On Diff #197085)

Can you use FS_explicit_specifier.isSpecified() here?

593–594 ↗(On Diff #197085)

Can you use = ExplicitSpecifier();?

clang/include/clang/Sema/Sema.h
10124 ↗(On Diff #197085)

Missing last word from this comment?

clang/include/clang/Serialization/ASTBitCodes.h
1444–1449 ↗(On Diff #197085)

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 ↗(On Diff #197085)

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

clang/lib/AST/DeclCXX.cpp
1905 ↗(On Diff #197085)

Default-construct.

2367 ↗(On Diff #197085)

Default-construct.

2370 ↗(On Diff #197085)

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

2540 ↗(On Diff #197085)

Default-construct.

clang/lib/Parse/ParseDecl.cpp
3532 ↗(On Diff #197085)

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

3536–3539 ↗(On Diff #197085)

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.

3898–3908 ↗(On Diff #197085)

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 ↗(On Diff #197085)

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 ↗(On Diff #197085)

Default-construct.

clang/lib/Sema/SemaDeclCXX.cpp
10880 ↗(On Diff #197085)

Loc here is unused; can you remove it?

11038 ↗(On Diff #197085)

Default-construct.

12611 ↗(On Diff #197085)

Default-construct.

12742 ↗(On Diff #197085)

Default-construct.

clang/lib/Sema/SemaInit.cpp
3817–3831 ↗(On Diff #197085)

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

3819 ↗(On Diff #196285)

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 ↗(On Diff #196285)

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 ↗(On Diff #197085)

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

384 ↗(On Diff #197085)

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

1754–1757 ↗(On Diff #197085)

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.

2067–2071 ↗(On Diff #197085)

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

clang/lib/Serialization/ASTReaderDecl.cpp
2021 ↗(On Diff #197085)

Please delete this.

clang/test/CXX/temp/temp.deduct.guide/p3.cpp
9–10 ↗(On Diff #197085)

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–1541 ↗(On Diff #197085)

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 ↗(On Diff #197085)

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
2033 ↗(On Diff #196285)

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
6174 ↗(On Diff #197085)

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

clang/lib/Sema/SemaInit.cpp
9361 ↗(On Diff #196285)

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–1541 ↗(On Diff #197085)

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
2604 ↗(On Diff #197378)

Please wrap this to 80 columns.

clang/include/clang/ASTMatchers/ASTMatchers.h
6174 ↗(On Diff #197085)

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.

6174 ↗(On Diff #197378)

Typo "FIXEME" -> "FIXME"

clang/lib/Parse/ParseDecl.cpp
3906 ↗(On Diff #197378)

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

clang/lib/Sema/SemaInit.cpp
9361 ↗(On Diff #196285)

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
2076 ↗(On Diff #197378)

Please reflow to 80 columns.

xbolva00 added inline comments.
clang/include/clang/AST/DeclBase.h
1534 ↗(On Diff #197378)

Small typo + update comment

Rakete1111 added inline comments.Apr 30 2019, 3:58 PM
clang/include/clang/AST/DeclCXX.h
2579 ↗(On Diff #197378)

Your or needs parens or the disambiguation is wrong.

clang/include/clang/Serialization/ASTReader.h
2435 ↗(On Diff #197378)

same here.

clang/lib/Sema/DeclSpec.cpp
952 ↗(On Diff #197378)

typo.

clang/test/CXX/temp/temp.deduct.guide/p1.cpp
74 ↗(On Diff #197043)

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 ↗(On Diff #197378)

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 ↗(On Diff #197378)

what is the issue

clang/lib/Sema/SemaInit.cpp
9361 ↗(On Diff #196285)

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 ↗(On Diff #197043)

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 ↗(On Diff #197378)

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 ↗(On Diff #197378)

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 ↗(On Diff #196285)

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 ↗(On Diff #196285)

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 ↗(On Diff #197722)

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 ↗(On Diff #198289)

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

@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

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