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

[Tyker via Phabricator via cfe-commits]

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



Comment at: cfe/trunk/lib/Parse/ParseDecl.cpp:3939
+ "both or neither of isAlreadyConsumed and 
"
+ "RangeEnd needs to be set");
+DS.SetRangeEnd(isAlreadyConsumed ? RangeEnd : Tok.getLocation());

RKSimon wrote:
> @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
i made a revision that fixes this: https://reviews.llvm.org/D61731


Repository:
  rL LLVM

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

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

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: cfe/trunk/lib/Parse/ParseDecl.cpp:3939
+ "both or neither of isAlreadyConsumed and 
"
+ "RangeEnd needs to be set");
+DS.SetRangeEnd(isAlreadyConsumed ? RangeEnd : Tok.getLocation());

@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


Repository:
  rL LLVM

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

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

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

could you commit this for me ?


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith reopened this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:373-374
+ ExplicitSpecifier ES, FunctionDecl *New) {
+  if (!ES.getExpr() || ES.getKind() != ExplicitSpecKind::Unresolved)
+return ES;
+  Expr *OldCond = ES.getExpr();

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 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` should fail 
with an error.)


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

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

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

yes i am on it.


Repository:
  rC Clang

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

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added a comment.

@Tyker This broke the Chromium build, could you investigate please? 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190506/270340.html


Repository:
  rC Clang

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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Also please update
https://clang.llvm.org/cxx_status.html


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Looks good, thank you!




Comment at: clang/lib/Sema/SemaInit.cpp:9359
   //   The converting constructors of T are candidate functions.
   if (Kind.isCopyInit() && !ListInit) {
 // Only consider converting constructors.

Please simplify this to `if (!AllowExplicit)`


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

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

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:9361
 // Only consider converting constructors.
-if (GD->isExplicit())
+if (!GD->isMaybeNotExplicit())
   continue;

rsmith wrote:
> Tyker wrote:
> > rsmith wrote:
> > > Tyker wrote:
> > > > rsmith wrote:
> > > > > Tyker wrote:
> > > > > > 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.
> > > > > Unlike in the previous case, we do //not// yet pass an 
> > > > > `AllowExplicit` flag into `AddTemplateOverloadCandidate` here, so 
> > > > > this will incorrectly allow dependent //explicit-specifier//s that 
> > > > > evaluate to `true` in copy-initialization contexts.
> > > > the default value for `AllowExplicit` is false. so the conversion will 
> > > > be removed in `AddTemplateOverloadCandidate`.
> > > 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?
> > 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
> > 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).
> > 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 struct X { X(int); };
> 
> explicit X(int) -> X; // #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
> explicit(E) X(int) -> X;
> ```
> 
> ... 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`.
there was an issue. now fixed.



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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:9361
 // Only consider converting constructors.
-if (GD->isExplicit())
+if (!GD->isMaybeNotExplicit())
   continue;

Tyker wrote:
> rsmith wrote:
> > Tyker wrote:
> > > rsmith wrote:
> > > > Tyker wrote:
> > > > > 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.
> > > > Unlike in the previous case, we do //not// yet pass an `AllowExplicit` 
> > > > flag into `AddTemplateOverloadCandidate` here, so this will incorrectly 
> > > > allow dependent //explicit-specifier//s that evaluate to `true` in 
> > > > copy-initialization contexts.
> > > the default value for `AllowExplicit` is false. so the conversion will be 
> > > removed in `AddTemplateOverloadCandidate`.
> > 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?
> 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
> 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).
> 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 struct X { X(int); };

explicit X(int) -> X; // #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
explicit(E) X(int) -> X;
```

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


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

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2579
+assert(
+!ES.getExpr() ||
+CXXConstructorDeclBits.HasTrailingExplicitSpecifier &&

Tyker wrote:
> Rakete wrote:
> > Your or needs parens or the disambiguation is wrong.
> i don't understand. but there is no need for disambiguation, `a && true == a` 
> so this will work regardless of operator priority.
Yeah sorry you're right, I meant that the parens are needed to shut up -Wparens 
warning about disambiguation.



Comment at: clang/include/clang/Serialization/ASTReader.h:2435
+uint64_t Kind = readInt();
+bool hasExpr = Kind & 0x1;
+Kind = Kind >> 1;

Tyker wrote:
> Rakete wrote:
> > same here.
> what is the issue
> For consistency with nearby code, please name this variable starting with a 
> capital letter.


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

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

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2579
+assert(
+!ES.getExpr() ||
+CXXConstructorDeclBits.HasTrailingExplicitSpecifier &&

Rakete wrote:
> Your or needs parens or the disambiguation is wrong.
i don't understand. but there is no need for disambiguation, `a && true == a` 
so this will work regardless of operator priority.



Comment at: clang/include/clang/Serialization/ASTReader.h:2435
+uint64_t Kind = readInt();
+bool hasExpr = Kind & 0x1;
+Kind = Kind >> 1;

Rakete wrote:
> same here.
what is the issue



Comment at: clang/lib/Sema/SemaInit.cpp:9361
 // Only consider converting constructors.
-if (GD->isExplicit())
+if (!GD->isMaybeNotExplicit())
   continue;

rsmith wrote:
> Tyker wrote:
> > rsmith wrote:
> > > Tyker wrote:
> > > > 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.
> > > Unlike in the previous case, we do //not// yet pass an `AllowExplicit` 
> > > flag into `AddTemplateOverloadCandidate` here, so this will incorrectly 
> > > allow dependent //explicit-specifier//s that evaluate to `true` in 
> > > copy-initialization contexts.
> > the default value for `AllowExplicit` is false. so the conversion will be 
> > removed in `AddTemplateOverloadCandidate`.
> 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?
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
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).



Comment at: clang/test/CXX/temp/temp.deduct.guide/p1.cpp:74
 virtual A(int(&)[28]) -> A; // expected-error {{'virtual' can only appear 
on non-static member functions}}
-const A(int(&)[28]) -> A; // expected-error {{deduction guide cannot be 
declared 'const'}}
+const A(int(&)[31]) -> A; // expected-error {{deduction guide cannot be 
declared 'const'}}
 

Rakete wrote:
> Is there a reason why you changed this?
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.


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

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2579
+assert(
+!ES.getExpr() ||
+CXXConstructorDeclBits.HasTrailingExplicitSpecifier &&

Your or needs parens or the disambiguation is wrong.



Comment at: clang/include/clang/Serialization/ASTReader.h:2435
+uint64_t Kind = readInt();
+bool hasExpr = Kind & 0x1;
+Kind = Kind >> 1;

same here.



Comment at: clang/lib/Sema/DeclSpec.cpp:952
+  ExplicitSpec.getExpr()) &&
+ "invalide ExplicitSpecifier");
   // 'explicit explicit' is ok, but warn as this is likely not what the user

typo.



Comment at: clang/test/CXX/temp/temp.deduct.guide/p1.cpp:74
 virtual A(int(&)[28]) -> A; // expected-error {{'virtual' can only appear 
on non-static member functions}}
-const A(int(&)[28]) -> A; // expected-error {{deduction guide cannot be 
declared 'const'}}
+const A(int(&)[31]) -> A; // expected-error {{deduction guide cannot be 
declared 'const'}}
 

Is there a reason why you changed this?


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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/include/clang/AST/DeclBase.h:1534
 
 /// 25 bits to fit in the remaining availible space.
 /// Note that this makes CXXConstructorDeclBitfields take

Small typo + update comment


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This is great, thank you so much!




Comment at: clang/include/clang/AST/DeclBase.h:1539-1541
+uint64_t NumCtorInitializers : 64 - NumDeclContextBits -
+NumFunctionDeclBits -
+/*Other used bits in CXXConstructorDecl*/ 3;

Tyker wrote:
> rsmith wrote:
> > I would prefer that we keep an explicit number here so that we can ensure 
> > that this field has the range we desire.
> 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 ?
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.



Comment at: clang/include/clang/AST/DeclCXX.h:2604
 
-  static CXXConstructorDecl *CreateDeserialized(ASTContext &C, unsigned ID,
-bool InheritsConstructor);
+  static CXXConstructorDecl *CreateDeserialized(ASTContext &C, unsigned ID, 
uint64_t AllocKind);
   static CXXConstructorDecl *

Please wrap this to 80 columns.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
 CXXConversionDecl)) {
+  // FIXEME : it's not clear whether this should match a dependent 
explicit()
   return Node.isExplicit();

Typo "FIXEME" -> "FIXME"



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
 CXXConversionDecl)) {
-  return Node.isExplicit();
+  return Node.getExplicitSpecifier().isSpecified();
 }

Tyker wrote:
> rsmith wrote:
> > 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()`.
> shouldn't this be able to match with deduction guides too ?
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.



Comment at: clang/lib/Parse/ParseDecl.cpp:3906
   } else
 Diag(Tok, DiagID) << PrevSpec;
 }

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



Comment at: clang/lib/Sema/SemaInit.cpp:9361
 // Only consider converting constructors.
-if (GD->isExplicit())
+if (!GD->isMaybeNotExplicit())
   continue;

Tyker wrote:
> rsmith wrote:
> > Tyker wrote:
> > > 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.
> > Unlike in the previous case, we do //not// yet pass an `AllowExplicit` flag 
> > into `AddTemplateOverloadCandidate` here, so this will incorrectly allow 
> > dependent //explicit-specifier//s that evaluate to `true` in 
> > copy-initialization contexts.
> the default value for `AllowExplicit` is false. so the conversion will be 
> removed in `AddTemplateOverloadCandidate`.
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?



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2076
+Method = CXXConstructorDecl::Create(
+SemaRef.Context, Record, StartLoc, NameInfo, T, TInfo, 
InstantiatedExplicitSpecifier,
+Constructor->isInlineSpecified(), false, Constructor->isConstexpr());

Please reflow to 80 columns.


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

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

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+  void setExplicitSpecifier(ExplicitSpecInfo ESI);
+

Tyker wrote:
> 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.
as it is only used by deserialization and ASTDeclReader is friend to all class 
with a setExplicitSpecifier.
I made setExplicitSpecifier private.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
 CXXConversionDecl)) {
-  return Node.isExplicit();
+  return Node.getExplicitSpecifier().isSpecified();
 }

rsmith wrote:
> 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()`.
shouldn't this be able to match with deduction guides too ?



Comment at: clang/lib/Sema/SemaInit.cpp:9361
 // Only consider converting constructors.
-if (GD->isExplicit())
+if (!GD->isMaybeNotExplicit())
   continue;

rsmith wrote:
> Tyker wrote:
> > 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.
> Unlike in the previous case, we do //not// yet pass an `AllowExplicit` flag 
> into `AddTemplateOverloadCandidate` here, so this will incorrectly allow 
> dependent //explicit-specifier//s that evaluate to `true` in 
> copy-initialization contexts.
the default value for `AllowExplicit` is false. so the conversion will be 
removed in `AddTemplateOverloadCandidate`.


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

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

[Tyker via Phabricator via cfe-commits]

Tyker marked 2 inline comments as done.
Tyker added inline comments.



Comment at: clang/include/clang/AST/DeclBase.h:1539-1541
+uint64_t NumCtorInitializers : 64 - NumDeclContextBits -
+NumFunctionDeclBits -
+/*Other used bits in CXXConstructorDecl*/ 3;

rsmith wrote:
> I would prefer that we keep an explicit number here so that we can ensure 
> that this field has the range we desire.
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 ?



Comment at: clang/lib/Sema/SemaInit.cpp:3817-3831
+if (AllowExplicit || !Conv->isExplicit()) {
   if (ConvTemplate)
-S.AddTemplateConversionCandidate(ConvTemplate, I.getPair(),
- ActingDC, Initializer, DestType,
- CandidateSet, AllowExplicit,
- /*AllowResultConversion*/false);
+S.AddTemplateConversionCandidate(
+ConvTemplate, I.getPair(), ActingDC, Initializer, DestType,
+CandidateSet, AllowExplicit, AllowExplicit,
+/*AllowResultConversion*/ false);
   else

rsmith wrote:
> We no longer pass `false` for `AllowExplicit` to `Add*Candidate` when 
> `CopyInitializing` is `true`. Is that an intentional change?
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.


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked 2 inline comments as done.
rsmith added a comment.

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




Comment at: clang/include/clang/AST/DeclBase.h:1539-1541
+uint64_t NumCtorInitializers : 64 - NumDeclContextBits -
+NumFunctionDeclBits -
+/*Other used bits in CXXConstructorDecl*/ 3;

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



Comment at: clang/include/clang/AST/DeclBase.h:1544-1548
+/// Wether this constructor has a trail-allocated explicit specifier.
+uint64_t hasTrailExplicit : 1;
+/// If this constructor does't have a trail-allocated explicit specifier.
+/// Wether this constructor is explicit specified.
+uint64_t isExplicitWhenNotTail : 1;

Typo "Wether" (x2).



Comment at: clang/include/clang/AST/DeclBase.h:1545-1548
+uint64_t hasTrailExplicit : 1;
+/// If this constructor does't have a trail-allocated explicit specifier.
+/// Wether this constructor is explicit specified.
+uint64_t isExplicitWhenNotTail : 1;

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



Comment at: clang/include/clang/AST/DeclCXX.h:1995
+  : ExplicitSpec(Expression, Flag) {}
+  ExplicitSpecFlag getFlag() const { return ExplicitSpec.getInt(); }
+  const Expr *getExpr() const { return ExplicitSpec.getPointer(); }

Usually we'd call this `Kind`, not `Flag`.



Comment at: clang/include/clang/AST/DeclCXX.h:2013
+  /// Return true if the ExplicitSpecifier isn't valid.
+  /// this state occurs after a substitution failures.
+  bool isInvalid() const {

"this" -> "This".



Comment at: clang/include/clang/AST/DeclCXX.h:2018-2020
+  void setExprAndFlag(Expr *E, ExplicitSpecFlag Flag) {
+ExplicitSpec.setPointerAndInt(E, Flag);
+  }

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



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6174
 CXXConversionDecl)) {
-  return Node.isExplicit();
+  return Node.getExplicitSpecifier().isSpecified();
 }

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



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2119
+def err_deduction_guide_redeclared : Error<
+  "redeclaration of a deduction guide">;
 def err_deduction_guide_specialized : Error<"deduction guide cannot be "

Drop the "a" here.



Comment at: clang/include/clang/Basic/Specifiers.h:26-28
+RESOLVED_FALSE,
+RESOLVED_TRUE,
+UNRESOLVED,

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



Comment at: clang/include/clang/Basic/Specifiers.h:31
+// this flag is used in combination with others.
+HAS_EXPR = 1 << 2
+  };

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



Comment at: clang/include/clang/Sema/DeclSpec.h:437
+Friend_specified(false), Constexpr_specified(false),
+FS_explicit_specifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE),
+Attrs(attrFactory), writtenBS(), ObjCQualifiers(nullptr) {}

Default-construct this.



Comment at: clang/include/clang/Sema/DeclSpec.h:572-574
+return FS_explicit_specifier.getFlag() !=
+   ExplicitSpecFlag::RESOLVED_FALSE ||
+   FS_explicit_specifier.getExpr();

Can you use `FS_explicit_specifier.isSpecified()` here?



Comment at: clang/include/clang/Sema/DeclSpec.h:593-594
 FS_virtualLoc = SourceLocation();
-FS_explicit_specified = false;
+FS_explicit_specifier =
+ExplicitSpecifier(nullptr, ExplicitSpecFlag::RESOLVED_FALSE);
 FS_explicitLoc = SourceLocation();

Can you use `= ExplicitSpecifier();`?



Comment at: clang/include/clang/Sema/Sema.h:10124
+  /// tryResolveExplicitSpecifier - Attempt to resolve the explict specifier.
+  /// Returns true if the explicit specifier is now.
+  bool tryResolveExplicitSpecifier(ExplicitSpecifier &ExplicitSpec);

Missing last word from this comment?

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

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

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.


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

[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 @Rakete.

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:
> Rakete wrote:
> > This is a useful diagnostic but you'll need to fix (a lot) of false 
> > positives:
> > 
> > ```
> > template  struct Foo { explicit(T{} +) Foo(); };
> > ```
> > 
> > gets me:
> > 
> > ```
> > main.cpp:1:50: error: expected expression
> > template  struct Foo { explicit(T{} +) Foo(); };
> >  ^
> > main.cpp:1:44: note: this expression is parsed as explicit(bool) since C++2a
> > template  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 ';'}}

Rakete 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-commit

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

[Gauthier via Phabricator via cfe-commits]

Tyker marked 2 inline comments as done.
Tyker added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+  void setExplicitSpecifier(ExplicitSpecInfo ESI);
+

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 ?



Comment at: clang/lib/Sema/SemaInit.cpp:3819
+AllowExplicit = AllowExplicit && !CopyInitializing;
+if (AllowExplicit || Conv->isMaybeNotExplicit()) {
   if (ConvTemplate)

rsmith wrote:
> Consider just removing this `if`. It's not clear that it carries its weight.
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 ?


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

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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:965
 auto *Conversion2 = cast(Method2);
-if (Conversion1->isExplicit() != Conversion2->isExplicit())
+if 
(!Conversion1->isEquivalentExplicit(Conversion2->getExplicitSpecifier()))
   return false;

martong wrote:
> 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.
`isEquivalentExplicit` calls `EquivalentExplicit`, which in turn calls 
`Stmt::Profile`.
Seems like `Stmt::Profile` relies on pointer equivalencies, however, here we 
may have two different `ASTContext`s 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.


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked an inline comment as done.
rsmith added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2022
+
+  bool isEquivalentExplicit(const CXXDeductionGuideDecl *Other) const;
+  bool isExplicit() const {

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



Comment at: clang/include/clang/AST/DeclCXX.h:2033
+
+  void setExplicitSpecifier(ExplicitSpecInfo ESI);
+

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



Comment at: clang/include/clang/AST/DeclCXX.h:2519
 
+  ExplicitSpecInfo ExplicitSpecifier;
+

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



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:37
+def warn_explicit_bool_breaking_change_cxx17 : Warning<
+  "this expression would be parsed as explicit(bool) in C++2a">
+  , InGroup, DefaultIgnore;

would be -> will be



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:38
+  "this expression would be parsed as explicit(bool) in C++2a">
+  , InGroup, DefaultIgnore;
+def note_explicit_bool_breaking_change_cxx2a : Note<

Nit: comma on previous line, please.



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">;
+

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



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2124-2129
+def err_deduction_guide_explicit_bool_mismatch : Error<
+  "the deduction guide is %select{not ||potentially }0explicit but "
+  "previous declaration was %select{not ||potentially }1explicit">;
+def err_unresolved_explicit_mismatch : Error<
+  "explicit specifier is not equivalent to the explicit specifier"
+  " from previous declaration">;

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



Comment at: clang/include/clang/Basic/Specifiers.h:27-29
+ESF_resolved_false,
+ESF_resolved_true,
+ESF_unresolved

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



Comment at: clang/include/clang/Sema/DeclSpec.h:376
+  using ExplicitSpecifierInfo =
+  llvm::PointerIntPair;
+

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.



Comment at: clang/lib/AST/DeclCXX.cpp:1867-1878
+static bool MaybeResolveExplicit(FunctionDecl *Decl, ExplicitSpecInfo &ESI) {
+  if (ESI.getInt() != ESF_unresolved || !ESI.getPointer())
+return true;
+  APValue Result;
+  if (ESI.getPointer()->EvaluateWithSubstitution(
+  Result, Decl->getASTContext(), Decl, llvm::ArrayRef())) {
+ESI.setInt(Result.getInt().getBoolValue() ? ESF_resolved_true

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.



Comment at: clang/lib/AST/DeclCXX.cpp:2514-2522
 bool CXXConstructorDecl::isConvertingConstructor(bool AllowExplicit) const {
   // C++ [class.conv.ctor]p1:
   //   A constructor declared with

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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D60934#1475908 , @Tyker wrote:

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


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

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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete 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  struct Foo { explicit(T{} +) Foo(); };
```

gets me:

```
main.cpp:1:50: error: expected expression
template  struct Foo { explicit(T{} +) Foo(); };
 ^
main.cpp:1:44: note: this expression is parsed as explicit(bool) since C++2a
template  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  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  struct Foo { explicit(a) Foo(); };
```

```
main.cpp:2:45: error: constexpr if condition is not a constant expression
template  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  struct Foo { explicit((T{}, false)) Foo(int); };

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



Comment at: clang/test/SemaCXX/cxx2a-compat.cpp:46
+#if __cplusplus <= 201703L
+// expected-warning@-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@-8 {{does not refer to a value}}
+// expected-error@-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



___
cfe-comm

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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3126
+auto Imp =
+importSeq(FromConstructor->getExplicitSpecifier().getPointer());
+if (!Imp)

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?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:965
 auto *Conversion2 = cast(Method2);
-if (Conversion1->isExplicit() != Conversion2->isExplicit())
+if 
(!Conversion1->isEquivalentExplicit(Conversion2->getExplicitSpecifier()))
   return false;

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.


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

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

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 195990.
Tyker added a comment.

@Quuxplusone @riccibruno  fixed / answered feedback


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

https://reviews.llvm.org/D60934

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/Overload.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/PCH/cxx-explicit-spec.cpp
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/explicit.cpp

Index: clang/test/SemaCXX/explicit.cpp
===
--- clang/test/SemaCXX/explicit.cpp
+++ clang/test/SemaCXX/explicit.cpp
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+
 namespace Constructor {
 struct A {
   A(int);
@@ -183,7 +185,8 @@
 const int ©List7 = {b};
 const int ©List8 = {n}; // expected-error {{no viable conversion}}
   }
-  
+
+#if __cplusplus < 201707L
   void testNew()
   {
 // 5.3.4p6:
@@ -200,7 +203,8 @@
 new int[i];
 new int[ni]; // expected-error {{array size expression of type 'NotInt' requires explicit conversion to type 'int'}}
   }
-  
+#endif
+
   void testDelete()
   {
 // 5.3.5pp2:
Index: clang/test/SemaCXX/cxx2a-explicit-bool.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-explicit-bool.cpp
@@ -0,0 +1,335 @@
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+
+template
+struct enable_ifv {};
+
+template
+struct enable_ifv {
+  static constexpr auto value = val;
+};
+
+template
+struct is_same {
+static constexpr bool value = false;
+};
+
+template
+struct is_same {
+static constexpr bool value = true;
+};
+
+namespace test0
+{
+
+template
+struct A {
+  explicit(1 << a)
+  //expected-error@-1 {{argument to explicit specifier is not a valid constant expression}}
+  //expected-note@-2 {{negative shift count -1}}
+  A(int);
+};
+
+A<-1> a(0); //expected-note {{in instantiation of template class}}
+
+template
+struct B {
+  explicit(b)
+  // expected-error@-1 {{use of undeclared identifier}}
+  // expected-note@-2 {{this expression is parsed as explicit(bool) since c++2a}}
+  B(int);
+};
+
+}
+
+namespace test1 {
+
+template
+struct A {
+  // expected-note@-1 {{candidate constructor}}
+  // expected-note@-2 {{candidate constructor}}
+  // expected-note@-3 {{candidate function}} expected-note@-3 {{candidate function}}
+  // expected-note@-4 {{candidate function}} expected-note@-4 {{candidate function}}
+  explicit(b) A(int, int = 0);
+  // expected-note@-1 {{explicit constructor declared here}}
+  // expected-note@-2 {{explicit constructor declared here}}
+  // expected-note@-3 {{explicit constructor declared here}}
+};
+
+template
+A::A(int, int) {}
+
+void f()
+{
+  A a0 = 0; // expected-error {{no viable conversion}}
+  A a1( 0);
+  A && a2 = 0;// expected-error {{could not bind}}
+  A && a3( 0);// expected-error {{could not bind}}
+  A a4{ 0};
+  A && a5 = { 0};// expected-error {{chosen constructor is explicit}}
+  A && a6{ 0};
+  A a7 = { 0}; // expected-error {{chosen constructor is explicit in copy-initialization}}
+
+  a0 = 0;
+  a1 = { 0}; // expected-error {{no viable overloaded '='}}
+  a2 = A( 0);
+  a3 = A{ 0};
+
+  A c0 =  ((short)0);
+  A c1( ((short)0));
+  A && c2 =  ((short)0);
+  A && c3( ((short)0));
+  A c4{ ((short)0)};
+  A && c5 = { ((short)0)};
+  A && c6{ ((short)0)};
+
+  A d1( 0, 0);
+  A d2{ 0, 0};
+  A d3 = { 0, 0}; // expected-error {{chosen constructor is explicit in copy-initialization}}
+
+  d1 = { 0, 0}; // expected-error {{no viable overloaded '='}}
+  d2 = A( 0, 0);
+  d3 = A{ 0, 0};
+}
+}
+
+namespace test2 {
+
+template
+struct A {
+// expected-note@-1 {{candidate constructor}} expected-note@-1 {{candidate constructor}}
+// expected-note@-2 {{candidate constructor}} expected-note@-2 {{candidate constructor}}
+  template
+  explicit(a ^ is_same::value)
+// expected-note@-1 {{explicit(bool) specifier resolved

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

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 195973.

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

https://reviews.llvm.org/D60934

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/Overload.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/PCH/cxx-explicit-spec.cpp
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/explicit.cpp

Index: clang/test/SemaCXX/explicit.cpp
===
--- clang/test/SemaCXX/explicit.cpp
+++ clang/test/SemaCXX/explicit.cpp
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+
 namespace Constructor {
 struct A {
   A(int);
@@ -183,7 +185,8 @@
 const int ©List7 = {b};
 const int ©List8 = {n}; // expected-error {{no viable conversion}}
   }
-  
+
+#if __cplusplus < 201707L
   void testNew()
   {
 // 5.3.4p6:
@@ -200,7 +203,8 @@
 new int[i];
 new int[ni]; // expected-error {{array size expression of type 'NotInt' requires explicit conversion to type 'int'}}
   }
-  
+#endif
+
   void testDelete()
   {
 // 5.3.5pp2:
Index: clang/test/SemaCXX/cxx2a-explicit-bool.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-explicit-bool.cpp
@@ -0,0 +1,335 @@
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+
+template
+struct enable_ifv {};
+
+template
+struct enable_ifv {
+  static constexpr auto value = val;
+};
+
+template
+struct is_same {
+static constexpr bool value = false;
+};
+
+template
+struct is_same {
+static constexpr bool value = true;
+};
+
+namespace test0
+{
+
+template
+struct A {
+  explicit(1 << a)
+  //expected-error@-1 {{argument to explicit specifier is not a valid constant expression}}
+  //expected-note@-2 {{negative shift count -1}}
+  A(int);
+};
+
+A<-1> a(0); //expected-note {{in instantiation of template class}}
+
+template
+struct B {
+  explicit(b)
+  // expected-error@-1 {{use of undeclared identifier}}
+  // expected-note@-2 {{this expression is parsed as explicit(bool) since c++2a}}
+  B(int);
+};
+
+}
+
+namespace test1 {
+
+template
+struct A {
+  // expected-note@-1 {{candidate constructor}}
+  // expected-note@-2 {{candidate constructor}}
+  // expected-note@-3 {{candidate function}} expected-note@-3 {{candidate function}}
+  // expected-note@-4 {{candidate function}} expected-note@-4 {{candidate function}}
+  explicit(b) A(int, int = 0);
+  // expected-note@-1 {{explicit constructor declared here}}
+  // expected-note@-2 {{explicit constructor declared here}}
+  // expected-note@-3 {{explicit constructor declared here}}
+};
+
+template
+A::A(int, int) {}
+
+void f()
+{
+  A a0 = 0; // expected-error {{no viable conversion}}
+  A a1( 0);
+  A && a2 = 0;// expected-error {{could not bind}}
+  A && a3( 0);// expected-error {{could not bind}}
+  A a4{ 0};
+  A && a5 = { 0};// expected-error {{chosen constructor is explicit}}
+  A && a6{ 0};
+  A a7 = { 0}; // expected-error {{chosen constructor is explicit in copy-initialization}}
+
+  a0 = 0;
+  a1 = { 0}; // expected-error {{no viable overloaded '='}}
+  a2 = A( 0);
+  a3 = A{ 0};
+
+  A c0 =  ((short)0);
+  A c1( ((short)0));
+  A && c2 =  ((short)0);
+  A && c3( ((short)0));
+  A c4{ ((short)0)};
+  A && c5 = { ((short)0)};
+  A && c6{ ((short)0)};
+
+  A d1( 0, 0);
+  A d2{ 0, 0};
+  A d3 = { 0, 0}; // expected-error {{chosen constructor is explicit in copy-initialization}}
+
+  d1 = { 0, 0}; // expected-error {{no viable overloaded '='}}
+  d2 = A( 0, 0);
+  d3 = A{ 0, 0};
+}
+}
+
+namespace test2 {
+
+template
+struct A {
+// expected-note@-1 {{candidate constructor}} expected-note@-1 {{candidate constructor}}
+// expected-note@-2 {{candidate constructor}} expected-note@-2 {{candidate constructor}}
+  template
+  explicit(a ^ is_same::value)
+// expected-note@-1 {{explicit(bool) specifier resolved to true}} expected-note@-1 {{explicit(bool) specifier resolved to true}}
+  

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

[Gauthier via Phabricator via cfe-commits]

Tyker marked 2 inline comments as done.
Tyker added inline comments.



Comment at: clang/lib/Sema/DeclSpec.cpp:959
+  // Each decl-specifier shall appear at most once in a complete
+  // decl-specifier-seq, except that long may appear twice.
+  if (hasExplicitSpecifier()) {

Quuxplusone wrote:
> 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)`?
I agree that it isn't clear. but i assumed that each counts every 
explicit-specifier as one.



Comment at: clang/test/SemaCXX/explicit.cpp:189
+
+#if __cplusplus < 201707L
   void testNew()

Quuxplusone wrote:
> Why?
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.
[[ https://godbolt.org/z/df_rvX | godbolt example ]]



Repository:
  rC Clang

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

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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

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


Repository:
  rC Clang

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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:2620
+
+  bool hasExplicitSpecifer() const {
+return ExplicitSpecifier.getInt() != ESF_resolved_false ||

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



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">;
+

"C++2a" should be uppercased.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2125
+def err_deduction_guide_explicit_bool : Error<
+  "explicit specfier of a deduction guide cannot depend on a constant 
expression">;
 def err_deduction_guide_specialized : Error<"deduction guide cannot be "

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



Comment at: clang/lib/Sema/DeclSpec.cpp:959
+  // Each decl-specifier shall appear at most once in a complete
+  // decl-specifier-seq, except that long may appear twice.
+  if (hasExplicitSpecifier()) {

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)`?



Comment at: clang/test/SemaCXX/explicit.cpp:189
+
+#if __cplusplus < 201707L
   void testNew()

Why?


Repository:
  rC Clang

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

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

[Gauthier via Phabricator via cfe-commits]

Tyker created this revision.
Tyker added a reviewer: rsmith.
Herald added a reviewer: martong.
Herald added a reviewer: shafik.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

this patch adds support for the explicit bool specifier.

added parsing for explicit bool specifier in ParseDecl.cpp
adapted storage of explicit specifier in AST and DeclSpec.

  was boolean value not it is a PointerIntPair with a flag and a potential 
expression.
  was stored in every function declaration. now it is stored only in 
CXXConstructorDecl, CXXDeductionGuideDecl and CXXConversionDecl.

adpated AddOverloadCandidate, AddTemplateOverloadCandidate, 
AddConversionCandidate and AddTemplateConversionCandidate
adapted template instatiation to instantiate explicit bool expressions.
to receive a boolean idicating if function resolved to be explicit should be 
removed.
adapted Serialization, ASTMatchers, ASTComparator and ASTPrinter
added test for semantic and serialization.

this patch is not yet complete. i still need to check the interaction with CTAD 
and deduction guides.
and add more tests for AST operations. but i wanted first feedback.

perhaps this patch should be splitted in smaller patchs. but making each patch 
testable as a standalone may be tricky.


Repository:
  rC Clang

https://reviews.llvm.org/D60934

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/Overload.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/PCH/cxx-explicit-spec.cpp
  clang/test/SemaCXX/cxx2a-explicit-bool.cpp
  clang/test/SemaCXX/explicit.cpp

Index: clang/test/SemaCXX/explicit.cpp
===
--- clang/test/SemaCXX/explicit.cpp
+++ clang/test/SemaCXX/explicit.cpp
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+
 namespace Constructor {
 struct A {
   A(int);
@@ -183,7 +185,8 @@
 const int ©List7 = {b};
 const int ©List8 = {n}; // expected-error {{no viable conversion}}
   }
-  
+
+#if __cplusplus < 201707L
   void testNew()
   {
 // 5.3.4p6:
@@ -200,7 +203,8 @@
 new int[i];
 new int[ni]; // expected-error {{array size expression of type 'NotInt' requires explicit conversion to type 'int'}}
   }
-  
+#endif
+
   void testDelete()
   {
 // 5.3.5pp2:
Index: clang/test/SemaCXX/cxx2a-explicit-bool.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-explicit-bool.cpp
@@ -0,0 +1,336 @@
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+
+template
+struct enable_ifv {};
+
+template
+struct enable_ifv {
+  static constexpr auto value = val;
+};
+
+template
+struct is_same {
+static constexpr bool value = false;
+};
+
+template
+struct is_same {
+static constexpr bool value = true;
+};
+
+namespace test0
+{
+
+template
+struct A {
+  explicit(1 << a)
+  //expected-error@-1 {{argument to explicit specifier is not a valid constant expression}}
+  //expected-note@-2 {{negative shift count -1}}
+  A(int);
+};
+
+A<-1> a(0); //expected-note {{in instantiation of template class}}
+
+template
+struct B {
+  explicit(b)
+  // expected-error@-1 {{use of undeclared identifier}}
+  // expected-note@-2 {{this expression is parsed as explicit(bool) since c++2a}}
+  B(int);
+};
+
+}
+
+
+namespace test1 {
+
+template
+struct A {
+  // expected-note@-1 {{candidate constructor}}
+  // expected-note@-2 {{candidate constructor}}
+  // expected-note@-3 {{candidate function}} expected-note@-3 {{candidate function}}
+  // expected-note@-4 {{candidate function}} expected-note@-4 {{candidate function}}
+  explicit(b) A(int, int = 0);
+  // expected-note@-1 {{explicit constructor declared here}}
+  // expected-note@-2 {{explicit constructor declared here}}
+  // expected-note@-3 {{explicit constructor declared here}}
+};
+
+template
+A::A(int, int) {}
+
+void f()
+{
+  A a0 = 0; // expected-error {{no viable conversion}}
+  A a1( 0);
+  A && a2 = 0;