[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/Lex/Lexer.cpp:3630-3632
 do {
-  ++CurPtr;
-} while (isHorizontalWhitespace(*CurPtr));
+  ++CurOffset;
+} while (isHorizontalWhitespace(BufferStart[CurOffset]));

davrec wrote:
> ```
> for (isHorizontalWhitespace(BufferStart[++CurOffset]);;)
>   ;
> ```
> might save a few instructions?  Worth trying since this function is the main 
> perf-critical one.
^ Ignore, erroneous :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Suggested a few adjustments in `LexTokenInternal` you might want to test for 
perf improvements (orthogonal to this patch, but could boost its numbers :).
And also noted that `Lexer::getBuffer()` has same issue as 
`getBufferLocation()` re potential for pointer invalidation IIUC.  At a minimum 
we need comments on these functions explaining any risks; better still to 
remove them from the public interface.  If downstream users use these functions 
and complain, good - they need to be aware of this change.




Comment at: clang/include/clang/Lex/Lexer.h:285
   /// Gets source code buffer.
-  StringRef getBuffer() const {
-return StringRef(BufferStart, BufferEnd - BufferStart);
-  }
+  StringRef getBuffer() const { return StringRef(BufferStart, BufferSize); }
 

Same issue as with `getBufferLocation()`, publicly returning it permits 
possible pointer invalidation. Fortunately I only see it used in a single spot 
(prior to this patch anyway) which can be easily eliminated IIUC.  Yank this 
function?  Or make private/append "Unsafe" to name (and explain in comments)?



Comment at: clang/include/clang/Lex/Lexer.h:307
   /// Return the current location in the buffer.
-  const char *getBufferLocation() const { return BufferPtr; }
+  const char *getBufferLocation() const {
+assert(BufferOffset <= BufferSize && "Invalid buffer state");

davrec wrote:
> I think I'd like this to return `unsigned`; i.e. I think after this patch we 
> should not even be publicly exposing buffer locations as pointers, IIUC.  A 
> brief search for uses of `getBufferLocation()` (there aren't many) suggests 
> this would be probably be fine and indeed would get rid of some unnecessary 
> pointer arithmetic.  And indeed if anything really needs that `const char *` 
> that might be a red flag to investigate further.
Looking more closely I see that `getCurrentBufferOffset` returns the unsigned, 
and this patch already changes some `getBufferLocation` usages to 
`getCurrentBufferOffset`.  So, I say either yank it or make it private or 
append "Unsafe" and explain in comments.



Comment at: clang/lib/Lex/Lexer.cpp:2948-2949
+  if (isHorizontalWhitespace(BufferStart[CurOffset])) {
+  SkipWhitespace(Result, CurOffset + 1, TokAtPhysicalStartOfLine);
+  return false;
   }

indent



Comment at: clang/lib/Lex/Lexer.cpp:2973-2975
+  char Char = getAndAdvanceChar(CurOffset, Tmp);
+  switch (Char) {
+  default:

indent



Comment at: clang/lib/Lex/Lexer.cpp:3630-3632
 do {
-  ++CurPtr;
-} while (isHorizontalWhitespace(*CurPtr));
+  ++CurOffset;
+} while (isHorizontalWhitespace(BufferStart[CurOffset]));

```
for (isHorizontalWhitespace(BufferStart[++CurOffset]);;)
  ;
```
might save a few instructions?  Worth trying since this function is the main 
perf-critical one.



Comment at: clang/lib/Lex/Lexer.cpp:3739-3752
+if (BufferStart[CurOffset] == '/' && BufferStart[CurOffset + 1] == '/' &&
+!inKeepCommentMode() && LineComment &&
+(LangOpts.CPlusPlus || !LangOpts.TraditionalCPP)) {
+  if (SkipLineComment(Result, CurOffset + 2, TokAtPhysicalStartOfLine))
 return true; // There is a token to return.
   goto SkipIgnoredUnits;
+} else if (BufferStart[CurOffset] == '/' &&

Spitballing again for possible minor perf improvements:
```
  if (char Char0 = BufferStart[CurOffset] == '/' && !inKeepCommentMode()) {
if (char Char1 = BufferStart[CurOffset + 1] == '/' && LineComment && 
(LangOpts.CPlusPlus || !LangOpts.TraditionalCPP)) {
  if (SkipLineComment(Result, CurOffset + 2, TokAtPhysicalStartOfLine))
return true; // There is a token to return.
  goto SkipIgnoredUnits;
} else if (Char1 == '*') {
  if (SkipBlockComment(Result, CurOffset + 2, TokAtPhysicalStartOfLine))
return true; // There is a token to return.
  goto SkipIgnoredUnits;
}
  } else if (isHorizontalWhitespace(Char0)) {
goto SkipHorizontalWhitespace;
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143142: [clang][lex] Enable Lexer to grow its buffer

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Only had a chance to give it a once over, I will look through more closely 
later, def by this weekend.  Main thing is I think we shouldn't be exposing the 
buffer pointers after this change, i.e. no public function should return `const 
char *`, unless I'm missing something.  If that box is checked and performance 
cost is negligible I'd give this the thumbs up.




Comment at: clang/include/clang/Lex/Lexer.h:307
   /// Return the current location in the buffer.
-  const char *getBufferLocation() const { return BufferPtr; }
+  const char *getBufferLocation() const {
+assert(BufferOffset <= BufferSize && "Invalid buffer state");

I think I'd like this to return `unsigned`; i.e. I think after this patch we 
should not even be publicly exposing buffer locations as pointers, IIUC.  A 
brief search for uses of `getBufferLocation()` (there aren't many) suggests 
this would be probably be fine and indeed would get rid of some unnecessary 
pointer arithmetic.  And indeed if anything really needs that `const char *` 
that might be a red flag to investigate further.



Comment at: clang/include/clang/Lex/Lexer.h:609
 
-  bool CheckUnicodeWhitespace(Token , uint32_t C, const char *CurPtr);
+  bool CheckUnicodeWhitespace(Token , uint32_t C, unsigned CurOffset);
 

FWIW it sucks that `uint32_t` is already sprinkled throughout the interface 
alongside `unsigned`, wish just one was used consistently, but that does not 
need to be addressed in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143142

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/DeclTemplate.h:3427
 
+TemplateParameterList *getReplacedTemplateParameterList(Decl *D);
+

mizvekov wrote:
> mizvekov wrote:
> > davrec wrote:
> > > davrec wrote:
> > > > I don't object with making this public, and I can see the argument for 
> > > > making this its own function rather than a Decl method all things being 
> > > > equal, but given that we already have 
> > > > `Decl::getDescribedTemplateParams`, I think that 
> > > > # this should probably also go in Decl,
> > > > # a good comment is needed explaining the difference between the two 
> > > > (e.g. that the `D` is the template/template-like decl itself, not a 
> > > > templated decl as required by `getDescribedTemplateParams`, or 
> > > > whatever), and what happens when it is called on a non-template decl, 
> > > > and
> > > > # it probably should be named just `getTemplateParams` or something 
> > > > that suggests its difference with `getDescribedTemplateParams`.
> > > > 
> > > On second thought this should definitely be a Decl method called 
> > > `getTemplateParameters()`, since all it does is dispatches to the derived 
> > > class's implementation of that.
> > I think this function shouldn't be public in that sense, it should only be 
> > used for implementation of retrieving parameter for Subst* nodes.
> > 
> > I don't think this should try to handle any other kinds of decls which 
> > can't appear as the AssociatedDecl in a Subst node.
> > 
> > There will be Subst specific changes here in the future, but which depend 
> > on some other enablers:
> > * We need to make Subst nodes for variable templates store the 
> > SpecializationDecl instead of the TemplateDecl, but this requires changing 
> > the order between creating the specialization and performing the 
> > substitution. I will do that in a separate patch.
> > * We will stop creating Subst* nodes for things we already performed 
> > substitution with sugar. And so, we won't need to handle alias templates, 
> > conceptdecls, etc. Subst nodes will only be used for things which we track 
> > specializations for, and that need resugaring.
> > 
> > It's only made 'public' because now we are using it across far away places 
> > like `Type.cpp` and `ExprCXX.cpp` and `TemplateName.cpp`.
> > 
> > I didn't think this needs to go as a Decl member, because of limited scope 
> > and because it only ever needs to access public members.
> > I don't think this justifies either a separate header to be included only 
> > where it's needed.
> > 
> > One option is to further make the name more specific, by adding `Internal` 
> > to the name for example.
> > Any other ideas?
> I ended up simply documenting that this is an internal function, for now.
That works for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/DeclTemplate.h:3427
 
+TemplateParameterList *getReplacedTemplateParameterList(Decl *D);
+

davrec wrote:
> I don't object with making this public, and I can see the argument for making 
> this its own function rather than a Decl method all things being equal, but 
> given that we already have `Decl::getDescribedTemplateParams`, I think that 
> # this should probably also go in Decl,
> # a good comment is needed explaining the difference between the two (e.g. 
> that the `D` is the template/template-like decl itself, not a templated decl 
> as required by `getDescribedTemplateParams`, or whatever), and what happens 
> when it is called on a non-template decl, and
> # it probably should be named just `getTemplateParams` or something that 
> suggests its difference with `getDescribedTemplateParams`.
> 
On second thought this should definitely be a Decl method called 
`getTemplateParameters()`, since all it does is dispatches to the derived 
class's implementation of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

I like the late changes, just need to add comments to the public methods, and 
maybe move `getReplacedTemplateParameterList` over to `Decl`.




Comment at: clang/include/clang/AST/DeclTemplate.h:3427
 
+TemplateParameterList *getReplacedTemplateParameterList(Decl *D);
+

I don't object with making this public, and I can see the argument for making 
this its own function rather than a Decl method all things being equal, but 
given that we already have `Decl::getDescribedTemplateParams`, I think that 
# this should probably also go in Decl,
# a good comment is needed explaining the difference between the two (e.g. that 
the `D` is the template/template-like decl itself, not a templated decl as 
required by `getDescribedTemplateParams`, or whatever), and what happens when 
it is called on a non-template decl, and
# it probably should be named just `getTemplateParams` or something that 
suggests its difference with `getDescribedTemplateParams`.




Comment at: clang/include/clang/AST/ExprCXX.h:4307
 
-  NonTypeTemplateParmDecl *getParameter() const {
-return ParamAndRef.getPointer();
+  Decl *getAssociatedDecl() const { return AssociatedDeclAndRef.getPointer(); }
+

Add comment



Comment at: clang/include/clang/AST/ExprCXX.h:4321
+
+  bool isReferenceParameter() const { return AssociatedDeclAndRef.getInt(); }
 

(Not your responsibility but while you're adding comments maybe you could add 
one for this too.)



Comment at: clang/include/clang/AST/ExprCXX.h:4378-4380
+  Decl *getAssociatedDecl() const { return AssociatedDecl; }
+
+  unsigned getIndex() const { return Index; }

Add comments



Comment at: clang/include/clang/AST/TemplateName.h:153-154
+
+  Decl *getAssociatedDecl() const { return AssociatedDecl; }
+  unsigned getIndex() const { return Bits.Index; }
 

Add comments



Comment at: clang/include/clang/AST/TemplateName.h:386-387
 public:
-  TemplateTemplateParmDecl *getParameter() const { return Parameter; }
+  Decl *getAssociatedDecl() const { return AssociatedDecl; }
+  unsigned getIndex() const { return Bits.Index; }
+

Add comments



Comment at: clang/lib/AST/DeclTemplate.cpp:1610
+  default:
+llvm_unreachable("Unhandled templated declaration kind");
+  }

Now that this is public, probably should return nullptr - otherwise users need 
the same huge switch just to decide whether they can call this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

Looks good, over to @ChuanqiXu




Comment at: clang/include/clang/Sema/Template.h:80
+struct ArgumentListLevel {
+  Decl *AssociatedDecl;
+  ArgList Args;

mizvekov wrote:
> davrec wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > Actually I think this one should be changed back to `ReplacedDecl` :)
> > > > ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make 
> > > > better sense in STTPT.
> > > I would be against introducing another term to refer to the same thing...
> > The reason we need this unfortunately vague term "AssociatedDecl" in STTPT 
> > is because it can either be a template/template-like declaration *or* a 
> > TemplateTypeParmDecl.  But here in MLTAL, it won't be a TTPD, will it?  It 
> > will always be the parent template/template-like declaration, right?  So 
> > there is no need for vagueness.  `ReplacedDecl` or `ParentTemplate` or 
> > something like that seems more appropriate.  
> No, it can be the TTPD which is used to represent the invented template for a 
> requires substitution.
K, makes sense to keep it the same then


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127695: [clang] Implement Template Specialization Resugaring

[David Rector via Phabricator via cfe-commits]

davrec added a subscriber: rjmccall.
davrec added a comment.

First thank you for having separated out the public AST changes into other 
patches, it makes these mostly-Sema changes much easier to review.

I don't see any major issues with the code, though this would benefit from a 
close look from more folks with deeper Sema familiarity.  Maybe @rjmccall would 
be willing or could @ someone?

The larger issues:

1. Performance - can we see some numbers?  and
2. There are a lot of FIXMEs introduced - understandable because of the scale, 
but it would be nice to hear you commit to eventually getting to those because 
otherwise they will probably remain indefinitely.   @aaron.ballman should 
probably weigh in/sign off on this one given the number of FIXMEs, which 
probably can't be handled before @mizvekov 's deadline (when is that again?).




Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:879-882
+  TypeLocBuilder TLB;
+  DecltypeTypeLoc DecltypeTL = TLB.push(T);
+  DecltypeTL.setDecltypeLoc(DS.getTypeSpecTypeLoc());
+  DecltypeTL.setRParenLoc(DS.getTypeofParensRange().getEnd());

Move back down?



Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:993-999
+  // Alias template specializations can produce types which are not valid
+  // nested name specifiers.
+  if (!T->isDependentType() && !T->getAs()) {
+Diag(TemplateNameLoc, diag::err_nested_name_spec_non_tag) << T;
+NoteAllFoundTemplates(Template);
+return true;
+  }

Move back up?



Comment at: clang/lib/Sema/SemaExpr.cpp:3427-3430
 if (unsigned BID = cast(VD)->getBuiltinID()) {
   if (!Context.BuiltinInfo.isDirectlyAddressable(BID)) {
 type = Context.BuiltinFnTy;
 valueKind = VK_PRValue;

Does this need the same changes as Sema::FixOverloadedFunctionReference?  (see 
below)



Comment at: clang/lib/Sema/SemaExpr.cpp:20800
 DRE->copyTemplateArgumentsInto(TemplateArgs);
+// FIXME: resugar
 return BuildDeclRefExpr(

Could this be done same as earlier (lines 19548-19553)?



Comment at: clang/lib/Sema/SemaExprMember.cpp:1862
 
 Qualifiers MemberQuals =
+Context.getCanonicalType(FieldType).getQualifiers();

Rename to FieldQuals



Comment at: clang/lib/Sema/SemaOverload.cpp:15357
 
 // FIXME: Duplicated from BuildDeclarationNameExpr.
+QualType Type;

I think this should have said "Duplicated from 
diagnoseUncapturableValueReferenceOrBinding" - if so do we need the below 
changes over there too?



Comment at: clang/lib/Sema/SemaTemplate.cpp:743
+  }
+  llvm_unreachable("");
+}

Add message



Comment at: clang/lib/Sema/SemaTemplate.cpp:95
 
+TemplateDecl *getTemplateDecl(NamedDecl *D) {
+  switch (D->getKind()) {

static/move to namespace {} below.  Or make it a public method of Decl?  
`Decl::getOriginalDescribedTemplate()` or something like that?  (If you go that 
route then best to fall back to getDescribedTemplate instead of the 
unreachable.)



Comment at: clang/lib/Sema/SemaTemplate.cpp:130
+// Maybe fall back to Decl::getDescribedTemplate.
+D->dumpColor();
+llvm_unreachable("Unhandled decl kind");

Not sure of policy, should this dump should be removed?  Several others below 
too.



Comment at: clang/lib/Sema/SemaTemplate.cpp:161
+public:
+  struct SemanticContextRAII {
+SemanticContextRAII(Resugarer , NamedDecl *ND,

Wouldn't hurt to add a brief comment above each RAII class describing its role



Comment at: clang/lib/Sema/SemaTemplate.cpp:257
+  assert(!Args.empty());
+  if (Reverse)
+R->CurTemplateToArgs.try_emplace(Template, Args);

Would be nice to have a comment explaining the effect of Reverse here or above 
addTypeToMap.  (Reverse iteration?)



Comment at: clang/lib/Sema/SemaTemplate.cpp:265
+  struct {
+NamedDecl *ND;
+const TemplateSpecializationType *TS;

Could clarify to CXXRecordDecl



Comment at: clang/lib/Sema/SemaTemplate.cpp:293
+  T->dump();
+  llvm_unreachable("");
+}

Add message



Comment at: clang/lib/Sema/SemaTemplate.cpp:466
+  Result = SemaRef.Context.getElaboratedType(
+  T->getKeyword(), QualifierLoc.getNestedNameSpecifier(), NamedT);
+

Maybe include T->OwnedTagDecl arg for consistency with deduction resugaring



Comment at: clang/lib/Sema/SemaTemplate.cpp:479
+const SubstTemplateTypeParmType *T = TL.getTypePtr();
+Decl *ReplacedDecl = T->getAssociatedDecl();
+Optional PackIndex = T->getPackIndex();

AssociatedDecl



Comment at: 

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D131858#3817646 , @mizvekov wrote:

> In D131858#3815057 , @davrec wrote:
>
>>> So this patch at least puts that into consistency.
>>
>> Is there already an analogous breakage for classes then?
>
> I am not sure I follow the question, but I meant that classes were already 
> deserealizing templates first, so no need to fix them.

Question was referring to @ChuanqiXu 's comment about this patch breaking 
modules for function importing, see his repro (which I haven't looked at 
closely).  I was asking if this meant that classes were already broken in a 
similar way, since you said this patch brings them into consistency.

>> Given @mizvekov's approaching deadline and @ChuanqiXu's limited 
>> availability, I'm inclined to think if @mizvekov can hack together a 
>> reasonable fix for this breakage, leaving a more rigorous/general solution 
>> for later as @ChuanqiXu suggests he would like to pursue, that would be 
>> sufficient to land this.  Does anyone disagree?
>
> Well I wouldn't say this fix is a hack any more than the whole code is, as 
> it's already playing by the rules of the existing implementation.
>
> We already have to deal with objects being accessed before they are fully 
> deserialized, this is part of the design of the current solution.
> The present patch just adds a, presumably new, case where the order is 
> important.
>
> Any changes in order to have a more strict, atomic deserialization are out of 
> scope here.

I think we misunderstand each other again, but are you saying the breakage 
found by @ChuanqiXu does not need to be fixed because it is out of scope?  (If 
classes are already broken due to this issue, the argument might have some 
validity.)

I wouldn't feel comfortable accepting this without either a) a hack to fix the 
breakage or b) @ChuanqiXu saying it is okay.  Maybe best to punt until 
@ChuanqiXu gets back from vacation in a couple weeks IIUC?




Comment at: clang/include/clang/AST/Type.h:5060
+  /// Gets the template parameter declaration that was substituted for.
+  const TemplateTypeParmDecl *getReplacedParameter() const;
+

mizvekov wrote:
> davrec wrote:
> > @sammccall , drafting you as a representative for downstream AST users, do 
> > you have any objection to changing this return type to a 
> > `TemplateTypeParmDecl` from a `TemplateTypeParmType`?  IIUC the change is 
> > not strictly necessary but is more for convenience, correct me if I'm wrong 
> > @mizvekov.  But I'm inclined to think STTPT is not much used downstream so 
> > the churn will be minimal/acceptable, am I wrong?
> I don't think this is strictly true.
> 
> We don't need to store the type, as that would increase storage for no 
> benefit. The type won't have more usable information than the associated 
> parameter declaration. The type can support a canonical representation, which 
> we don't need.
> 
> We will store the template-like declaration + index, and simply access the 
> parameter decl from there.
> Otherwise creating a type from that requires the ASTContext and would 
> possibly involve allocation.
I see, so it is a necessary change, and anyone who really depends on this 
returning the TTPT will need to convert it via 
`Context.getTemplateTypeParmType(...)`.  But any code written 
getReplacementParameter()->getDecl() would just lose the getDecl() and be fine. 
 And since all the info is in decl most people will only need to do the latter. 
 Given the necessity I think that's okay.



Comment at: clang/include/clang/Sema/Template.h:80
+struct ArgumentListLevel {
+  Decl *AssociatedDecl;
+  ArgList Args;

mizvekov wrote:
> davrec wrote:
> > Actually I think this one should be changed back to `ReplacedDecl` :)
> > ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make 
> > better sense in STTPT.
> I would be against introducing another term to refer to the same thing...
The reason we need this unfortunately vague term "AssociatedDecl" in STTPT is 
because it can either be a template/template-like declaration *or* a 
TemplateTypeParmDecl.  But here in MLTAL, it won't be a TTPD, will it?  It will 
always be the parent template/template-like declaration, right?  So there is no 
need for vagueness.  `ReplacedDecl` or `ParentTemplate` or something like that 
seems more appropriate.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

[David Rector via Phabricator via cfe-commits]

davrec added a comment.



> I will give an example using a Typedef for exposition, but the same 
> motivation applies with UsingType.
>
> Say we have this code:
>
>   template  struct A { using type1 = T; };
>   using Int = int;
>   
>   using type2 = A::type1;
>
> See this example live (with that resugaring patch) at: 
> https://godbolt.org/z/jP64Pern8
>
> We want the underlying type of `type2` to have that `Int` sugar. It would 
> also be good if we could keep the TypedefType sugar referencing the 
> instantiation of `type1` from the `A` template.
>
> The problem here is that the TypedefType is currently limited to having the 
> declaration's underlying type, which is just from an instantiation of 
> `A`, and knows nothing about camel case `Int`.
> This is similar to the infamous problem with `std::string` turning into 
> `std::basic_string` in templates.
>
> We could emit a new TypedefDecl with the underlying type that we want, but 
> creating declarations is expensive, so we want to avoid that.
> We could create a new AST node that represented more accurately what was 
> happening. But the question is, is this worth the cost?
> Do you see use cases for this yourself?
>
> We want resugaring to be cheap and introduce as little changes in the AST as 
> possible, to get a better chance of affording to have it to be on all the 
> time.
> If we introduce new nodes with more information, that might increase the cost 
> and complexity, and it's hard to justify without an use case.

This explanation and example are very helpful.  Parts of them probably should 
make it into the documentation for `isDivergent()` or whatever it is called.

More importantly the documentation needs to emphasize that when `isDivergent()` 
is true, the underlying type really is more meaningful than the decl, and 
should be relied on wherever possible.  After all the reason they differ *for 
now* is that the underlying type may not have an associated decl, simply due to 
the cost concerns of introducing such a decl.  But it is possible in the future 
we could change course and decide to introduce the implicit TypedefDecls or 
UsingDecls.  Then they would no longer be divergent, and it is the underlying 
decl that would change, not the underlying type.

Whether `isDivergent()` is the right name, vs. `resugared()`, or 
`hasLessCanonicalizedType()` or `hasMoreSpecificType()` as @erichkeane 
suggested (all seem reasonable), is less important to me than just documenting 
that method well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133468

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added a subscriber: sammccall.
davrec added a comment.

My concerns have been addressed, except for some more nitpicking on names which 
I think are uncontroversial.

Regarding the modules breakage found by @ChuanqiXu:

> Well we touch FunctionTemplates and VariableTemplates in this patch, because 
> they were not doing template first.
> For whatever reason, class templates were already doing template first, so no 
> need to fix that.
>
> So this patch at least puts that into consistency.

Is there already an analogous breakage for classes then?

Given @mizvekov's approaching deadline and @ChuanqiXu's limited availability, 
I'm inclined to think if @mizvekov can hack together a reasonable fix for this 
breakage, leaving a more rigorous/general solution for later as @ChuanqiXu 
suggests he would like to pursue, that would be sufficient to land this.  Does 
anyone disagree?




Comment at: clang/include/clang/AST/ASTContext.h:1621-1623
+Decl *ReplacedDecl, unsigned Index,
 Optional PackIndex) const;
+  QualType getSubstTemplateTypeParmPackType(Decl *ReplacedDecl, unsigned Index,

AssociatedDecl



Comment at: clang/include/clang/AST/Type.h:5060
+  /// Gets the template parameter declaration that was substituted for.
+  const TemplateTypeParmDecl *getReplacedParameter() const;
+

@sammccall , drafting you as a representative for downstream AST users, do you 
have any objection to changing this return type to a `TemplateTypeParmDecl` 
from a `TemplateTypeParmType`?  IIUC the change is not strictly necessary but 
is more for convenience, correct me if I'm wrong @mizvekov.  But I'm inclined 
to think STTPT is not much used downstream so the churn will be 
minimal/acceptable, am I wrong?



Comment at: clang/include/clang/AST/TypeProperties.td:730
   }
+  def : Property<"replacedDecl", DeclRef> {
+let Read = [{ node->getAssociatedDecl() }];

"associatedDecl"



Comment at: clang/include/clang/AST/TypeProperties.td:743
 return ctx.getSubstTemplateTypeParmType(
-cast(replacedParameter),
-replacementType, PackIndex);
+replacementType, replacedDecl, Index, PackIndex);
   }]>;

associatedDecl



Comment at: clang/include/clang/AST/TypeProperties.td:762
 let Class = SubstTemplateTypeParmPackType in {
-  def : Property<"replacedParameter", QualType> {
-let Read = [{ QualType(node->getReplacedParameter(), 0) }];
+  def : Property<"replacedDecl", DeclRef> {
+let Read = [{ node->getAssociatedDecl() }];

"associatedDecl"



Comment at: clang/include/clang/AST/TypeProperties.td:774
 return ctx.getSubstTemplateTypeParmPackType(
- cast(replacedParameter),
-replacementPack);
+replacedDecl, Index, replacementPack);
   }]>;

associatedDecl



Comment at: clang/include/clang/Sema/Template.h:80
+struct ArgumentListLevel {
+  Decl *AssociatedDecl;
+  ArgList Args;

Actually I think this one should be changed back to `ReplacedDecl` :)
ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make better 
sense in STTPT.



Comment at: clang/include/clang/Sema/Template.h:163
+/// cases might even be a template parameter itself.
+Decl *getAssociatedDecl(unsigned Depth) const {
+  assert(NumRetainedOuterLevels <= Depth && Depth < getNumLevels());

getReplacedDecl



Comment at: clang/include/clang/Sema/Template.h:205
 /// list.
-void addOuterTemplateArguments(const TemplateArgumentList *TemplateArgs) {
-  addOuterTemplateArguments(ArgList(TemplateArgs->data(),
-TemplateArgs->size()));
+void addOuterTemplateArguments(Decl *AssociatedDecl, ArgList Args) {
+  assert(!NumRetainedOuterLevels &&

ReplacedDecl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

[David Rector via Phabricator via cfe-commits]

davrec added a subscriber: sammccall.
davrec added a comment.

> In general, forgetting about just the above particular way of creating them, 
> there might still exist only one TypedefDecl in the whole program, which 
> every TypedefType is referencing, and they can still be divergent, ie have 
> different sugared underlying type.

Can you give a code example demonstrating such a case, and any other 
distinctive cases where isDivergent() will be true?

>> But these types are a different matter:
>>
>>   auto t29 = 0 ? (RPX1){} : (RPY1){};
>>   auto t32 = 0 ? (UPX1){} : (UPY1){};
>>
>> In the process of unifying the sugar to get the deduced type of t29/t32, we 
>> will compare their sugar, which has the same basic structure for each, but 
>> different decls/found decls; so unifying these types will simply involve 
>> unifying their decls; and getCommonDecl(X,Y) has been defined to will simply 
>> get the older declaration between them.  So, the TypedefType/UsingType sugar 
>> in t29/t32 will reference the X1 decls and friends, *not* the Y1s, as its 
>> Decls/FoundDecls.  (Right?)
>
> But again, we are discussing one particular mechanism of creating these 
> divergent types.
> How that mechanism works was defined by a previous patch.
>
> But I don't think the choice between declarations is arbitrary. That type 
> unification mechanism has one basic principle, when two properties of a type 
> are different, we pick the one that is closest to the canonical property, 
> (except in some very special circumstances where the standard requires 
> something different).
> So the choice for the older decl is principled, the canonical decl will be 
> the oldest one. If that wasn't the case, we would need some other choice.



> I don't think the declaration picked is arbitrary, and again they are the 
> "Same" declaration, so picking null would be throwing out more information 
> than is necessary for no principled reason.

Hmm.  Redeclarations never change semantic information, and in general don't 
change sugar information, so the older one is indeed the common one from any 
principled standpoint - but as your test cases demonstrate, in the case of 
typedef and using-decl redeclarations, the sugar information can be changed.  
Does this result in some arbitrariness in choosing the older decl as the common 
decl?  My first instinct is that it does.  But as I think about it further...I 
think I agree with you, it is still principled to choose the earlier 
using-decl/typedef decl (though an argument can be made for choosing the very 
first decl, not just the earlier one, for typedefs and using-decls).

But, if we agree the decl choice is principled, the next question: what is the 
different principle behind choosing a different underlying type?   Why can't 
the underlying type for `t29`/`t32` be sugar for X1 and friends too?

To state the issue more practically: when users of the AST see 
isDivergent()==true, what do they do?  Which path should they take to fetch 
information from this node?

If we absolutely have to allow these types to diverge, we need very good 
documentation that helps them answer this.

Wrangling in @sammccall here since he developed `UsingType` and so might be 
affected/need clarity on how to handle `isDivergent()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133468

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132816: [clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

Agree this needs a brief commit message, just explaining that previously the 
underlying type had to be canonical, but now it can be sugared, allowing a 
better/more informative representation.




Comment at: clang/include/clang/AST/TypeProperties.td:738
   def : Creator<[{
 // The call to getCanonicalType here existed in ASTReader.cpp, too.
 return ctx.getSubstTemplateTypeParmType(

^ Delete this comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132816

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

If I understand this correctly: whenever isDivergent() is true, `getDecl()` or 
`getFoundDecl()` is an arbitrary choice between multiple candidate 
(re)declarations of a typedef or using decl.  In particular it will be the 
first redeclaration, //even though each different redeclaration can contain 
different sugar information//.

E.g. considering your examples:

  using X1 = int;
  using Y1 = int;
  
  using RPB1 = X1*;
  using RPX1 = RPB1;
  using RPB1 = Y1*; // redeclared
  using RPY1 = RPB1;
  
  namespace A { using type1 = X1*; };
  namespace C { using A::type1; };
  using UPX1 = C::type1;
  namespace A { using type1 = Y1*; };  // redeclared
  namespace C { using A::type1; }; // redeclared
  using UPY1 = C::type1;

So far so good: isDivergent() would be false for all the above TypedefTypes and 
UsingTypes, since the UnderlyingTypes and Decls/FoundDecls are consistent.  But 
these types are a different matter:

  auto t29 = 0 ? (RPX1){} : (RPY1){};
  auto t32 = 0 ? (UPX1){} : (UPY1){};

In the process of unifying the sugar to get the deduced type of t29/t32, we 
will compare their sugar, which has the same basic structure for each, but 
different decls/found decls; so unifying these types will simply involve 
unifying their decls; and getCommonDecl(X,Y) has been defined to will simply 
get the older declaration between them.  So, the TypedefType/UsingType sugar in 
t29/t32 will reference the X1 decls and friends, *not* the Y1s, as its 
Decls/FoundDecls.  (Right?)

The UnderlyingType however will not be as arbitrary, as it will use 
getCommonType to skip over everything the two don't have in common.

If I have this right, my question is: since the Decl/FoundDecl is an arbitrary 
choice, should we solve this by either

1. constructing these with a null Decl/FoundDecl for such types, or if that is 
problematic,
2. renaming `isDivergent` to `declIsArbitrary()` or something like that, to 
suggest that not only do the underlying type and decl diverge, it is the 
underlying type which is more meaningful, not the decl.






Comment at: clang/include/clang/AST/Type.h:4490
   UsingShadowDecl *getFoundDecl() const { return Found; }
+  bool isDivergent() const { return UsingBits.isDivergent; }
   QualType getUnderlyingType() const;

The name of this might be less important than just documenting it extremely 
well here, with an example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133468

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:
\
+llvm_unreachable("Unexpected " Kind ": " #Class);
+

aaronpuchert wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > mizvekov wrote:
> > > > davrec wrote:
> > > > > mizvekov wrote:
> > > > > > davrec wrote:
> > > > > > > mizvekov wrote:
> > > > > > > > davrec wrote:
> > > > > > > > > Could we just return `X` here? Would that just default to the 
> > > > > > > > > old behavior instead of crashing whenever unforeseen cases 
> > > > > > > > > arise?  
> > > > > > > > No, I think we should enforce the invariants and make sure we 
> > > > > > > > are handling everything that can be handled.
> > > > > > > > 
> > > > > > > > Classing `TemplateTypeParm`  as sugar-free was what was wrong 
> > > > > > > > and we missed this on the review.
> > > > > > > There might always going to be a few rare corner cases vulnerable 
> > > > > > > to this though, particularly as more types are added and the 
> > > > > > > people adding them don't pay strict attention to how to 
> > > > > > > incorporate them here, and don't write the requisite tests (which 
> > > > > > > seem very difficult to foresee and produce).  When those cases 
> > > > > > > arise we will be crashing even though we could produce a 
> > > > > > > perfectly good program with the intended semantics; the only 
> > > > > > > thing that would suffer for most users is slightly less clear 
> > > > > > > diagnostic messages for those rare cases.  I think it would be 
> > > > > > > better to let those cases gradually percolate to our attention 
> > > > > > > via bug reports concerning those diagnostics, rather than 
> > > > > > > inconveniencing the user and demanding immediate attention via 
> > > > > > > crashes.  Or am I missing something?
> > > > > > I could on the same argument remove all asserts here and just let 
> > > > > > the program not crash on unforeseen circumstances.
> > > > > > 
> > > > > > On the other hand, having these asserts here helps us catch bugs 
> > > > > > not only here, but in other parts of the code. For example uniquing 
> > > > > > / canonicalization bugs.
> > > > > > 
> > > > > > If someone changes the properties of a type so that the assumptions 
> > > > > > here are not valid anymore, it's helpful to have that pointed out 
> > > > > > soon.
> > > > > > 
> > > > > > Going for as an example this specific bug, if there werent those 
> > > > > > asserts / unreachables in place and we had weakened the validation 
> > > > > > here, it would take a very long time for us to figure out we were 
> > > > > > making the wrong assumption with regards to TemplateTypeParmType.
> > > > > > 
> > > > > > I'd rather figure that out sooner on CI / internal testing rather 
> > > > > > than have a bug created by a user two years from now.
> > > > > Yes its nicer to developers to get stack traces and reproductions 
> > > > > whenever something goes wrong, and crashing is a good way to get 
> > > > > those, but users probably won't be so thrilled about it.  Especially 
> > > > > given that the main selling point of this patch is that it makes 
> > > > > diagnostics nicer for users: isn't it a bit absurd to crash whenever 
> > > > > we can't guarantee our diagnostics will be perfect?
> > > > > 
> > > > > And again the real problem is future types not being properly 
> > > > > incorporated here and properly tested: i.e. the worry is that this 
> > > > > will be a continuing source of crashes, even if we handle all the 
> > > > > present types properly.
> > > > > 
> > > > > How about we leave it as an unreachable for now, to help ensure all 
> > > > > the present types are handled, but if months or years from now there 
> > > > > continue to be crashes due to this, then just return X (while maybe 
> > > > > write something to llvm::errs() to encourage users to report it), so 
> > > > > we don't make the perfect the enemy of the good.
> > > > It's not about crashing when it won't be perfect. We already do these 
> > > > kinds of things, see the FIXMEs around the TemplateArgument and 
> > > > NestedNameSpecifier, where we just return something worse than we wish 
> > > > to, just because we don't have time to implement it now.
> > > > 
> > > > These unreachables and asserts here are about testing / documenting our 
> > > > knowledge of the system and making it easier to find problems. These 
> > > > should be impossible to happen in correct code, and if they do happen 
> > > > because of mistakes, fixing them sooner is just going to save us 
> > > > resources.
> > > > 
> > > > `llvm::errs` suggestion I perceive as out of line with current practice 
> > > > in LLVM, we don't and have a system for producing diagnostics for 
> > > > results possibly affected by FIXME and TODO situations and the like, as 
> > > > far as I know, and I am not aware of any discussions 

[PATCH] D111509: [clang] use getCommonSugar in an assortment of places

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1113-1114
+// "double _Complex" is promoted to "long double _Complex".
+static QualType handleComplexFloatConversion(Sema , ExprResult ,
+ QualType LHSType, QualType 
RHSType,
+ bool PromotePrecision) {

davrec wrote:
> Rename params for clarity, e.g.
> LHS->Longer
> LHSType->LongerType
> RHSType->ShorterType
> 
Actually I probably have that backwards, I think LHS is the Shorter 
expression...in any case you see why renaming would be helpful


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111509: [clang] use getCommonSugar in an assortment of places

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

The lines you changed (clang/lib/Sema/SemaExpr.cpp:1091-1150) look good (IIUC 
you just change cast to cast_as and dyn_cast to getAs, and reorganize for 
clarity).  I suggested some renaming and a few more comments to further clarify.




Comment at: clang/lib/Sema/SemaExpr.cpp:1113-1114
+// "double _Complex" is promoted to "long double _Complex".
+static QualType handleComplexFloatConversion(Sema , ExprResult ,
+ QualType LHSType, QualType 
RHSType,
+ bool PromotePrecision) {

Rename params for clarity, e.g.
LHS->Longer
LHSType->LongerType
RHSType->ShorterType




Comment at: clang/lib/Sema/SemaExpr.cpp:1143
 return RHSType;
 
   int Order = S.Context.getFloatingTypeOrder(LHSType, RHSType);

```
// Compute the rank of the two types, regardless of whether they are complex.
```



Comment at: clang/lib/Sema/SemaExpr.cpp:1145
   int Order = S.Context.getFloatingTypeOrder(LHSType, RHSType);
-
-  auto *LHSComplexType = dyn_cast(LHSType);
-  auto *RHSComplexType = dyn_cast(RHSType);
-  QualType LHSElementType =
-  LHSComplexType ? LHSComplexType->getElementType() : LHSType;
-  QualType RHSElementType =
-  RHSComplexType ? RHSComplexType->getElementType() : RHSType;
-
-  QualType ResultType = S.Context.getComplexType(LHSElementType);
-  if (Order < 0) {
-// Promote the precision of the LHS if not an assignment.
-ResultType = S.Context.getComplexType(RHSElementType);
-if (!IsCompAssign) {
-  if (LHSComplexType)
-LHS =
-S.ImpCastExprToType(LHS.get(), ResultType, CK_FloatingComplexCast);
-  else
-LHS = S.ImpCastExprToType(LHS.get(), RHSElementType, CK_FloatingCast);
-}
-  } else if (Order > 0) {
-// Promote the precision of the RHS.
-if (RHSComplexType)
-  RHS = S.ImpCastExprToType(RHS.get(), ResultType, CK_FloatingComplexCast);
-else
-  RHS = S.ImpCastExprToType(RHS.get(), LHSElementType, CK_FloatingCast);
-  }
-  return ResultType;
+  if (Order < 0)
+return handleComplexFloatConversion(S, LHS, LHSType, RHSType,

```
// Promote the precision of the LHS if not an assignment.
```



Comment at: clang/lib/Sema/SemaExpr.cpp:1147
+return handleComplexFloatConversion(S, LHS, LHSType, RHSType,
+/*PromotePrecision=*/!IsCompAssign);
+  return handleComplexFloatConversion(S, RHS, RHSType, LHSType,

```
// Promote the precision of the RHS unless it is already the same as the LHS.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111509

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:
\
+llvm_unreachable("Unexpected " Kind ": " #Class);
+

mizvekov wrote:
> davrec wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > Could we just return `X` here? Would that just default to the old 
> > > > behavior instead of crashing whenever unforeseen cases arise?  
> > > No, I think we should enforce the invariants and make sure we are 
> > > handling everything that can be handled.
> > > 
> > > Classing `TemplateTypeParm`  as sugar-free was what was wrong and we 
> > > missed this on the review.
> > There might always going to be a few rare corner cases vulnerable to this 
> > though, particularly as more types are added and the people adding them 
> > don't pay strict attention to how to incorporate them here, and don't write 
> > the requisite tests (which seem very difficult to foresee and produce).  
> > When those cases arise we will be crashing even though we could produce a 
> > perfectly good program with the intended semantics; the only thing that 
> > would suffer for most users is slightly less clear diagnostic messages for 
> > those rare cases.  I think it would be better to let those cases gradually 
> > percolate to our attention via bug reports concerning those diagnostics, 
> > rather than inconveniencing the user and demanding immediate attention via 
> > crashes.  Or am I missing something?
> I could on the same argument remove all asserts here and just let the program 
> not crash on unforeseen circumstances.
> 
> On the other hand, having these asserts here helps us catch bugs not only 
> here, but in other parts of the code. For example uniquing / canonicalization 
> bugs.
> 
> If someone changes the properties of a type so that the assumptions here are 
> not valid anymore, it's helpful to have that pointed out soon.
> 
> Going for as an example this specific bug, if there werent those asserts / 
> unreachables in place and we had weakened the validation here, it would take 
> a very long time for us to figure out we were making the wrong assumption 
> with regards to TemplateTypeParmType.
> 
> I'd rather figure that out sooner on CI / internal testing rather than have a 
> bug created by a user two years from now.
Yes its nicer to developers to get stack traces and reproductions whenever 
something goes wrong, and crashing is a good way to get those, but users 
probably won't be so thrilled about it.  Especially given that the main selling 
point of this patch is that it makes diagnostics nicer for users: isn't it a 
bit absurd to crash whenever we can't guarantee our diagnostics will be perfect?

And again the real problem is future types not being properly incorporated here 
and properly tested: i.e. the worry is that this will be a continuing source of 
crashes, even if we handle all the present types properly.

How about we leave it as an unreachable for now, to help ensure all the present 
types are handled, but if months or years from now there continue to be crashes 
due to this, then just return X (while maybe write something to llvm::errs() to 
encourage users to report it), so we don't make the perfect the enemy of the 
good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:
\
+llvm_unreachable("Unexpected " Kind ": " #Class);
+

mizvekov wrote:
> davrec wrote:
> > Could we just return `X` here? Would that just default to the old behavior 
> > instead of crashing whenever unforeseen cases arise?  
> No, I think we should enforce the invariants and make sure we are handling 
> everything that can be handled.
> 
> Classing `TemplateTypeParm`  as sugar-free was what was wrong and we missed 
> this on the review.
There might always going to be a few rare corner cases vulnerable to this 
though, particularly as more types are added and the people adding them don't 
pay strict attention to how to incorporate them here, and don't write the 
requisite tests (which seem very difficult to foresee and produce).  When those 
cases arise we will be crashing even though we could produce a perfectly good 
program with the intended semantics; the only thing that would suffer for most 
users is slightly less clear diagnostic messages for those rare cases.  I think 
it would be better to let those cases gradually percolate to our attention via 
bug reports concerning those diagnostics, rather than inconveniencing the user 
and demanding immediate attention via crashes.  Or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:
\
+llvm_unreachable("Unexpected " Kind ": " #Class);
+

Could we just return `X` here? Would that just default to the old behavior 
instead of crashing whenever unforeseen cases arise?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/Type.h:5530-5537
 /// Represents a type that was referred to using an elaborated type
 /// keyword, e.g., struct S, or via a qualified name, e.g., N::M::type,
 /// or both.
 ///
 /// This type is used to keep track of a type name as written in the
 /// source code, including tag keywords and any nested-name-specifiers.
 /// The type itself is always "sugar", used to express what was written

This documentation needs to be updated given the changes in this patch.  
Suggest you remove the first paragraph and flesh out the second paragraph, e.g.

```
/// A sugar type used to keep track of a type name as written in the
/// source code, including any tag keywords (e.g. struct S) and/or any 
/// nested-name-specifiers (e.g. N::M::type).  Note it will even be created
/// for types written *without* tag words or nested-name-specifiers, to
/// properly represent their absence in the written code.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: WIP: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D128113#3777353 , @mizvekov wrote:

> @davrec  @alexfh
>
> I finally managed to have a talk to @rsmith about this.
>
> He thinks that, even if we can't come up with a better solution in time, the 
> benefits of this patch justify the costs observed, as those substitution 
> nodes are pretty useless without a way to index the pack, and having a rich 
> AST is one of Clang's main goals.

I support that - re-merge for now, and consider ways to reduce costs going 
forward.

I thought this through every which way and could not avoid @mizvekov's 
conclusion, that the only other way would be another Subst* type node, from 
which the pack indices could be inferred, but only via non-straightforward code 
requiring a bird's eye view of the AST.

Taking a step back and reconsidering, it's clear the original way - just 
storing the pack index - is very much preferable to all that complexity.  The 
benefits justify the costs for most users, and for clang developers who would 
be burdened by the appearance of an obscure new type class.  And as I said 
previously I think broad flags governing how much sugar/non-semantic info is 
added to the AST, i.e. letting users balance benefits vs. costs for themselves, 
is the better way to handle these concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127695: WIP: clang: Implement Template Specialization Resugaring

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:534-540
+  QualType TransformSubstTemplateTypeParmType(TypeLocBuilder ,
+  SubstTemplateTypeParmTypeLoc TL) 
{
+QualType QT = TL.getType();
+const SubstTemplateTypeParmType *T = TL.getTypePtr();
+Decl *ReplacedDecl = T->getReplacedDecl();
+
+Optional PackIndex = T->getPackIndex();

davrec wrote:
> Haven't thought this through fully, but: would the following make D128113 
> (storing the pack index in the STTPT or introducing a new sugar type) 
> unnecessary?
> ```
> map, Optional> CurPackIndices;
> QualType TransformSubstTemplateTypeParmType(TypeLocBuilder ,
>   SubstTemplateTypeParmTypeLoc 
> TL) {
>QualType QT = TL.getType();
>const SubstTemplateTypeParmType *T = TL.getTypePtr();
>Decl *ReplacedDecl = T->getReplacedDecl();
>
>Optional  = CurPackIndices[{ReplacedDecl, 
> T->getIndex()}];
>if (!PackIndex && T->getReplacedParameter()->isParameterPack())
>  PackIndex = 0;
> 
>...
> 
>if (PackIndex)
>  ++PackIndex;
>  // ^ maybe reset to zero if > pack size, if we might be resugaring 
> multiple expansions
>return QT;
> }
> ```
Disregard above - upon further thought this does not improve the things; there 
still isn't enough info about the substitutions.  I.e. the issue is with 
substitutions, not the parameter declarations for which they are substituted.  
So a sugar node wrapping the STTPTs to represent each expansion instance really 
is needed.  Then when we have that I think we could map from those to their 
current pack indices per the above to infer the pack indices.

For this sugar node, maybe we could just modify `SubstTemplateTypeParmPackType` 
so it is not just canonical, but can also be sugar wrapping substituted STTPTs, 
as opposed to introducing a new Subst* type class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127695

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/Type.h:5000
+
+  const TemplateTypeParmDecl *getReplacedParameter() const;
+

Another question worth raising: is it acceptable churn to change the return of 
`getReplacedParameter()` from a `TemplateTypeParmType` to a 
`TemplateTypeParmDecl`?  I have no opinion, but maybe others want to weigh in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:1618
+  QualType getSubstTemplateTypeParmType(QualType Replacement,
+Decl *ReplacedDecl,
+unsigned Index) const;

mizvekov wrote:
> davrec wrote:
> > ReplacedDecl -> ReplacedParamParent (see below)
> We need a more apt name.
> 
> How about `ReplacedTemplateLikeEntityDecl`?
Problem is if it's a `TemplateTypeParmDecl` its not even a template like 
entity.  That's why I lean towards "AssociatedDecl" or something like that, 
that is just so vague users are forced to look up the documentation to 
understand it.



Comment at: clang/include/clang/AST/Type.h:4998
+  /// Gets the templated entity that was substituted.
+  Decl *getReplacedDecl() const { return ReplacedDecl; }
+

mizvekov wrote:
> davrec wrote:
> > To reiterate an earlier comment which may have been overlooked: I think 
> > this needs a clearer name and documentation (so long as I do not 
> > misunderstand what kinds of decls it may be - see other inline comment).  
> > 
> > Given that we have `getReplacedParameter()`, I think something like 
> > `getReplacedParamParent()` makes the most sense.
> > 
> > And the documentation should suggest the relationship between 
> > `getReplacedParameter` and this (i.e. `getReplacedParamParent()` + 
> > `getIndex()` -> `getReplacedParameter()`).  
> > 
> > Plus, add back in the original documentation for `getReplacedParameter()`, 
> > and a brief line of documentation for `getIndex()` (and do same for the 
> > Pack version below too).
> > 
> Yeah like I said, we can't suggest a relationship very much, I don't think we 
> should try to explain how to obtain the parameter from the main entity, that 
> would be exposing too much guts.
> 
> We just offer a method to get it, and hope for the best.
> 
> The name for this method, whatever we pick, will probably be a bit vague 
> sounding, because in Clang a template is a very vague concept (in the 
> philosophical sense and in the C++20 sense).
Let's say we call it `getAssociatedDecl()`.  Whatever the name I strongly think 
we need some good documentation explaining what these return, if they are going 
to be public methods.  How about something like this:
```
  /// Gets the template parameter declaration that was substituted for.
  const TemplateTypeParmDecl *getReplacedParameter() const;

  /// If the replaced TemplateTypeParmDecl belongs to a parent template/ 
  /// template-like declaration, returns that parent.
  /// Otherwise, returns the replaced TTPD.
  Decl *getAssociatedDecl() const { return AssociatedDecl; }
  
  /// If the replaced parameter belongs to a parent template/template-like  
  /// declaration, returns the index of the parameter in that parent.
  /// Otherwise, returns zero.
  unsigned getIndex() const { return SubstTemplateTypeParmTypeBits.Index; }
```





Comment at: clang/lib/AST/Type.cpp:3709
+unsigned Index) {
+  if (const auto *TTP = dyn_cast(D))
+return TTP;

mizvekov wrote:
> davrec wrote:
> > Will this cast ever succeed?  I.e. are there still places 
> > `Context.getSubstTemplateTypeParmType(...)` is called with a TTPD as the 
> > `ReplacedDecl`?  That would make `getReplacedDecl()` much more 
> > complicated/less understandable - can we change any such places to always 
> > pass the parent template/templated entity as the ReplacedDecl, for 
> > consistency (and so that we can rename ReplacedDecl to ReplacedParamParent)?
> Yeah, there is this one place related to concepts where we invent a 
> substitution and there is actually no associated declaration at all, besides 
> just the invented template parameter, so in this case the TTP is the parent 
> and parameter.
> 
> So that is why I picked such a vaguely named acessor... because there is no 
> order here, anarchy reigns.
I see.  So close.  I agree we need to go vague then.  I think ReplacedDecl 
isn't quite vague enough though; a user could easily conflate 
getReplacedParameter and getReplacedDecl.  I think you hit on a better term in 
your wording though: "AssociatedDecl".  So long as it is documented I think 
that could work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

I hope I'm not stepping on any toes given the recent changes in code ownership, 
but I'm accepting this and D130308  because

1. They are a nice improvement to the AST that naturally follows from the 
earlier work adding sugar to type deductions,
2. I have given it a fairly close look and it LGTM,
3. @rsmith has taken a step back and so may not be available to give it a final 
look,
4. @mizvekov has verified that the performance impact is negligible, and
5. Other reviewers seem to have at least given it a once over and have not 
identified major issues.

Anyone object/need more time to review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133262: [clang] Represent __make_integer_seq as alias template in the AST

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

LGTM aside from a nit




Comment at: clang/include/clang/AST/DeclTemplate.h:455
 
+  bool isTypeAlias() const;
+

Add doc, e.g. "Whether this is a written or built-in type alias template."

And nit: maybe move this above the `classof` functions, since those and other 
boilerplate functions are usually the last public members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133262

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127695: WIP: clang: Implement Template Specialization Resugaring

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:534-540
+  QualType TransformSubstTemplateTypeParmType(TypeLocBuilder ,
+  SubstTemplateTypeParmTypeLoc TL) 
{
+QualType QT = TL.getType();
+const SubstTemplateTypeParmType *T = TL.getTypePtr();
+Decl *ReplacedDecl = T->getReplacedDecl();
+
+Optional PackIndex = T->getPackIndex();

Haven't thought this through fully, but: would the following make D128113 
(storing the pack index in the STTPT or introducing a new sugar type) 
unnecessary?
```
map, Optional> CurPackIndices;
QualType TransformSubstTemplateTypeParmType(TypeLocBuilder ,
  SubstTemplateTypeParmTypeLoc TL) {
   QualType QT = TL.getType();
   const SubstTemplateTypeParmType *T = TL.getTypePtr();
   Decl *ReplacedDecl = T->getReplacedDecl();
   
   Optional  = CurPackIndices[{ReplacedDecl, 
T->getIndex()}];
   if (!PackIndex && T->getReplacedParameter()->isParameterPack())
 PackIndex = 0;

   ...

   if (PackIndex)
 ++PackIndex;
 // ^ maybe reset to zero if > pack size, if we might be resugaring 
multiple expansions
   return QT;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127695

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

This looks good, except for the confusion around the name `getReplacedDecl` - I 
would really like the public AST methods to be clearly named and documented, so 
please address that.

Thanks for doing the performance tests.  It may create some additional unique 
STTPTs, but not to do with pack expansion, i.e. not to the extent that created 
problems for D128113 , so I think performance 
concerns have been addressed.

And, it seems possible that the additional info added to STTPT here might make 
D128113  (storing pack index or introducing a 
new sugar type to help infer it) unnecessary for resugaring purposes - I will 
add an inline comment to https://reviews.llvm.org/D127695 to show you what I 
mean/ask about this.

(I'm tempted to hit accept but a. I think there is at least one other issue 
raised by another reviewer which hasn't been addressed and b. I'm unsure of the 
etiquette and don't want to step on toes particularly given the recent changes 
to code ownership.)




Comment at: clang/include/clang/AST/ASTContext.h:1618
+  QualType getSubstTemplateTypeParmType(QualType Replacement,
+Decl *ReplacedDecl,
+unsigned Index) const;

ReplacedDecl -> ReplacedParamParent (see below)



Comment at: clang/include/clang/AST/Type.h:4998
+  /// Gets the templated entity that was substituted.
+  Decl *getReplacedDecl() const { return ReplacedDecl; }
+

To reiterate an earlier comment which may have been overlooked: I think this 
needs a clearer name and documentation (so long as I do not misunderstand what 
kinds of decls it may be - see other inline comment).  

Given that we have `getReplacedParameter()`, I think something like 
`getReplacedParamParent()` makes the most sense.

And the documentation should suggest the relationship between 
`getReplacedParameter` and this (i.e. `getReplacedParamParent()` + `getIndex()` 
-> `getReplacedParameter()`).  

Plus, add back in the original documentation for `getReplacedParameter()`, and 
a brief line of documentation for `getIndex()` (and do same for the Pack 
version below too).




Comment at: clang/lib/AST/Type.cpp:3709
+unsigned Index) {
+  if (const auto *TTP = dyn_cast(D))
+return TTP;

Will this cast ever succeed?  I.e. are there still places 
`Context.getSubstTemplateTypeParmType(...)` is called with a TTPD as the 
`ReplacedDecl`?  That would make `getReplacedDecl()` much more complicated/less 
understandable - can we change any such places to always pass the parent 
template/templated entity as the ReplacedDecl, for consistency (and so that we 
can rename ReplacedDecl to ReplacedParamParent)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D128113#3753656 , @davrec wrote:

> In D128113#3753640 , @mizvekov 
> wrote:
>
>> In D128113#3753624 , @davrec wrote:
>>
>>> Or just `SubstTemplateTypeParmType` could store this number in addition to 
>>> its `TemplateTypeParmType`?  (E.g. the first Ts in an expansion is 0, the 
>>> second Ts in the same expansion is 1, etc. - but it resets for the next 
>>> expansion.)
>>
>> Well that number is just the pack_index, as implemented in this current 
>> patch :)

Disregard my previous comment, you're right, that would be identical.  I'm 
mixing up my STTPTs and TTPTs :).  So the proposed solution would be to make 
the TTPTs unique, and map from the TTPTs to their current pack indices in the 
Resugarer.  But, that is probably identical in terms of memory usage as your 
proposal to introduce a new sugar type representing unique expansion instances, 
and the latter is probably clearer/less disruptive.

Thanks for your work on this, and for explaining these complexities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D128113#3753640 , @mizvekov wrote:

> In D128113#3753624 , @davrec wrote:
>
>> Or just `SubstTemplateTypeParmType` could store this number in addition to 
>> its `TemplateTypeParmType`?  (E.g. the first Ts in an expansion is 0, the 
>> second Ts in the same expansion is 1, etc. - but it resets for the next 
>> expansion.)
>
> Well that number is just the pack_index, as implemented in this current patch 
> :)

I meant that, in an of expansion `(Ts, Ts, Us)`, either the two `Ts` would be 
two unique `TemplateTypeParmType`s by adding an instanceIndex or whatever to 
refer to the two different syntactic instances (this would of course naturally 
result in two unique STTPTs), or for a bit more savings the TTPTs for the two 
`Ts` could continue to be the same but their STTPTs would include the 
instanceIndex, so there is only one unique TTPT but two unique STTPTs referring 
to `Ts`.

I.e. if there are 100 types in in `Ts` there would still only be two unique 
`SubstTemplateTypeParmTypes` created, as opposed to only 1 as there currently 
is, or 100 as this patch creates.

Any way it is done, the goal would be to make the STTPTs just as unique as 
necessary so the Resugarer can keep map from the unique STTPTs to its current 
pack index, and iterate the relevant index with each call to 
`TransformSubstTemplateTypeParmType`.

If I'm missing something and it is substantially more complex than that, then 
you're right that storing the pack index might end up the best solution - we 
don't want to e.g. have to do considerable Sema logic in the Resugarer just to 
infer the pack index.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Great examples.

> Check this example out: https://godbolt.org/z/rsGsM6GrM
>
>   template  struct A {
> template  struct B {
> using type1 = void ((void (*...fps)(Ts, Us)));
> };
>   };
>   using type2 = A::B::type1;
>
>   TypeAliasDecl 0x55ffe8b45368  col:7 type2 'A char>::B::type1':'void ((void (*)(int, short), void (*)(char, 
> bool)))'
> `-ElaboratedType 0x55ffe8b452f0 'A::B::type1' 
> sugar
>   `-TypedefType 0x55ffe8b452d0 'A::B::type1' sugar
> |-TypeAlias 0x55ffe8b45258 'type1'
> `-FunctionProtoType 0x55ffe8b451e0 'void ((void (*)(int, short), void 
> (*)(char, bool)))' cdecl
>   |-ParenType 0x55ffe8b12150 'void' sugar
>   | `-BuiltinType 0x55ffe8ac6370 'void'
>   |-PointerType 0x55ffe8b44550 'void (*)(int, short)'
>   | `-ParenType 0x55ffe8b444f0 'void (int, short)' sugar
>   |   `-FunctionProtoType 0x55ffe8b444b0 'void (int, short)' cdecl
>   | |-BuiltinType 0x55ffe8ac6370 'void'
>   | |-SubstTemplateTypeParmType 0x55ffe8b44310 'int' sugar
>   | | |-TemplateTypeParmType 0x55ffe8b11440 'Ts' dependent 
> contains_unexpanded_pack depth 0 index 0 pack
>   | | | `-TemplateTypeParm 0x55ffe8b113c0 'Ts'
>   | | `-BuiltinType 0x55ffe8ac6410 'int'
>   | `-SubstTemplateTypeParmType 0x55ffe8b443c0 'short' sugar
>   |   |-TemplateTypeParmType 0x55ffe8b11960 'Us' dependent 
> contains_unexpanded_pack depth 1 index 0 pack
>   |   | `-TemplateTypeParm 0x55ffe8b118d8 'Us'
>   |   `-BuiltinType 0x55ffe8ac63f0 'short'
>   `-PointerType 0x55ffe8b450c0 'void (*)(char, bool)'
> `-ParenType 0x55ffe8b45060 'void (char, bool)' sugar
>   `-FunctionProtoType 0x55ffe8b447d0 'void (char, bool)' cdecl
> |-BuiltinType 0x55ffe8ac6370 'void'
> |-SubstTemplateTypeParmType 0x55ffe8b44630 'char' sugar
> | |-TemplateTypeParmType 0x55ffe8b11440 'Ts' dependent 
> contains_unexpanded_pack depth 0 index 0 pack
> | | `-TemplateTypeParm 0x55ffe8b113c0 'Ts'
> | `-BuiltinType 0x55ffe8ac63b0 'char'
> `-SubstTemplateTypeParmType 0x55ffe8b446e0 'bool' sugar
>   |-TemplateTypeParmType 0x55ffe8b11960 'Us' dependent 
> contains_unexpanded_pack depth 1 index 0 pack
>   | `-TemplateTypeParm 0x55ffe8b118d8 'Us'
>   `-BuiltinType 0x55ffe8ac6390 'bool'

If it were as simple as the above case the Resugarer TreeTransform could say 
keep a map of `TemplateTypeParmType`s to current pack indices, and iterate 
those during each each `TransformSubstTemplateTypeParmType` call, but...

> And in fact, a given parameter pack might be referenced more than one time in 
> a given pack expansion pattern:
>
>   template  struct A {
> template  struct B {
> using type1 = void ((void (*...fps)(Ts, Ts, Us)));
> };
>   };
>
> Ie those two Ts are referencing the same argument within the pack, we can't 
> confuse that with an expansion.
>
> So think also about a case like the above, but where you are performing the 
> expansion just one time. It doesn't look like to me you can figure that out 
> from what Clang leaves behind in the AST at the moment.

...the different `Ts` are the same `TemplateTypeParmType`, so that wouldn't 
work.  You're right, more info is needed.

> We may need to create a new AST node, which is like a Subst variant of a 
> PackExpansionType, at the expansion loci.

Or, maybe `TemplateTypeParmType` could store a number that would make it unique 
for each *instance* of it in a given expansion?  Or just 
`SubstTemplateTypeParmType` could store this number in addition to its 
`TemplateTypeParmType`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Two last thoughts:
1: To reiterate it doesn't seem right to view this (or any patch adding 
not-semantically-relevant info to the AST) as a one-size-fits-all costs vs. 
benefits optimization.  On the one hand, the additional AST info hurts 
compilation performance.  On the other, the info might make debugging faster, 
or be useful to a tool.  A flag governing how much non-semantically-necessary 
info to add to the AST really seems the better solution in general, and 
warrants more discussion in cases like this (e.g. Matheus's other resugaring 
patches, which may cause this debate to resurface regardless of the approach he 
takes here).  The user could set the flag high when debugging, and decrease it 
when diagnostics/debug info are less expected/not needed.  (In light of the 
performance tests done here, the performance benefit of allowing the user to 
omit *other* non-semantically-necessary nodes from the AST could be 
significant; such a flag could allow that.)

2: After thinking about it further I don't think the pack index provides 
sufficiently useful info in any case, since packs will always be expanded in 
full, in order: when you find the first `SubstTemplateTypeParmType` expanded 
from a pack, the rest are sure to be right behind.  IIUC I see how including 
the pack index makes resugaring more straightforward: the substitution of the 
sugared info for the non-sugared info occurs via a TreeTransform (see 
clang/lib/Sema/SemaTemplate.cpp in https://reviews.llvm.org/D127695), and by 
storing the pack index Matheus can simply override 
`TransformSubstTemplateTypeParmType` to make use of the pack index to easily 
fetch the corresponding sugared info.  But since the pack indices are 
predictable given a bird's eye view of the AST, maybe state info can be stored 
in the TreeTransform to allow the pack index to be inferred in each call to 
`TransformSubstTemplateTypeParmType`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12767
+Ctx.getQualifiedType(Underlying),
+::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl()));
+  }

mizvekov wrote:
> davrec wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > This last argument should probably be omitted/nullptr passed, since
> > > > # We probably won't encounter any owned tag decls in the types passed 
> > > > to this function;
> > > > # Even if we did, it would not necessarily be true that they both have 
> > > > non-null owned tag decls, and I don't see any nullptr checks in 
> > > > getCommonDecl; and
> > > > # Even if we checked for nullptr there, and say passed the same 
> > > > argument to X and Y so EX==EY, and that had an owned tag decl, it is 
> > > > not clear to me it would be appropriate to construct a new type with 
> > > > the same owned tag decl as another type.
> > > 1. We can see, a one liner like this will do: `int e = true ? (struct 
> > > S*)0 : (struct S*)0;`
> > > 
> > > Though normally in an example like the above, you would only see the 
> > > owned decl on X, because when we get to Y the name has already been 
> > > introduced into the scope.
> > > 
> > > I have an idea for a test case, but it's a bit convoluted:
> > > 
> > > This also introduces an OwnedTagDecl: `(struct S*)0`.
> > > 
> > > So with resugaring, if we resugar T, it might be possible to construct 
> > > this situatiation. Given if it's appropriate to keep the OwnedTagDecl 
> > > when resugaring, of course, which goes to 3)
> > > 
> > > 2. This is handled by `declaresSameEntity`, if either or both decls are 
> > > null, we say they are not the same decl.
> > > 
> > > 3. That I am not sure. I see that this OwnedTagDecl is only used by the 
> > > type printer, and it replaces printing the rest of the type by printing 
> > > the OwnedDecl instead. So why do you think it would not be appropriate 
> > > that the rebuilt type keeps this?
> > (Re #3) Because I think the sense ownership is respected in how 
> > ownedTagDecls are presently handled: at most one ElaboratedType owns a 
> > particular TagDecl, *and* that type does not seem to be reused elsewhere in 
> > the AST.  (This is relied on in https://reviews.llvm.org/D131685, which was 
> > on my mind when I looked at this.)
> > 
> > E.g. consider this expansion of your example:
> > ```
> > auto e = true ? (struct S*)0 : (true ? (struct S*)0 : (struct S*)0);
> > ```
> > The first ElaboratedType has an ownedTagDecl; the second is a distinct 
> > ElaboratedType without an ownedTagDecl, and the third equals the second.  
> > 
> > Now in practice what this means is that `getCommonDecl` when used on binary 
> > expressions of this sort will always be nullptr, so no harm done.  But 
> > suppose it is called with X=Y=some ET with an ownedTagDecl (which is the 
> > only time I believe we could see a non null getCommonDecl result here).  
> > Should the type that is returned have that same owned tag decl?  I think 
> > that would violate the sense of the original type "owning" that decl that 
> > seems to be respected elsewhere in the AST.
> > 
> > Re type printing, I believe that ownedTagDecls only affects printing of 
> > defined TagDecls like `struct S { int i; } x;` (which is what I was mostly 
> > thinking re ownedTagDecls - I don't think we will see those here); for 
> > undefined ones like `struct S x;`, printing should be unaffected whether 
> > there is an ownedTagDecl or not.
> > 
> With type deduction this can happen:
> 
> ```
> auto x = (struct S*)0; // it will also appear in the type of x
> using t = decltype(x); // and now also in t
> ```
> 
> With your second example, it can also happen:
> 
> ```
> struct S { int i; } x;
> int e = true ?  : (struct S*)0;
> ```
> 
> In those cases, the type node that owns the decl will be the same object, but 
> that is only because of uniquing.
> 
> It may be possible that two different objects end up in this situation, if 
> for example they come from different modules that got merged.
> With type deduction this can happen:
> ```
> auto x = (struct S*)0; // it will also appear in the type of x
> using t = decltype(x); // and now also in t
> ```

My local clang is out of date - the type of `x` for me is just an AutoType 
wrapping a PointerType to a RecordType.  In light of the addition of the 
ElaboratedType there, it seems fine to be consistent here with however that 
case handles ownedTagDecls, but FWIW I do not think that deduced ElaboratedType 
should have an ownedTagDecl either.  Only the original type should be the owner 
- not any type deduced from it.

This is a minor issue with few if any observable effects for now, but should be 
kept in mind if issues arise later: the fact the ownership of ownedTagDecls is 
respected was the only factor that made 
https://discourse.llvm.org/t/ast-parent-of-class-in-a-friend-declaration/64275 
easily solvable.

> With your second 

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12767
+Ctx.getQualifiedType(Underlying),
+::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl()));
+  }

mizvekov wrote:
> davrec wrote:
> > This last argument should probably be omitted/nullptr passed, since
> > # We probably won't encounter any owned tag decls in the types passed to 
> > this function;
> > # Even if we did, it would not necessarily be true that they both have 
> > non-null owned tag decls, and I don't see any nullptr checks in 
> > getCommonDecl; and
> > # Even if we checked for nullptr there, and say passed the same argument to 
> > X and Y so EX==EY, and that had an owned tag decl, it is not clear to me it 
> > would be appropriate to construct a new type with the same owned tag decl 
> > as another type.
> 1. We can see, a one liner like this will do: `int e = true ? (struct S*)0 : 
> (struct S*)0;`
> 
> Though normally in an example like the above, you would only see the owned 
> decl on X, because when we get to Y the name has already been introduced into 
> the scope.
> 
> I have an idea for a test case, but it's a bit convoluted:
> 
> This also introduces an OwnedTagDecl: `(struct S*)0`.
> 
> So with resugaring, if we resugar T, it might be possible to construct this 
> situatiation. Given if it's appropriate to keep the OwnedTagDecl when 
> resugaring, of course, which goes to 3)
> 
> 2. This is handled by `declaresSameEntity`, if either or both decls are null, 
> we say they are not the same decl.
> 
> 3. That I am not sure. I see that this OwnedTagDecl is only used by the type 
> printer, and it replaces printing the rest of the type by printing the 
> OwnedDecl instead. So why do you think it would not be appropriate that the 
> rebuilt type keeps this?
(Re #3) Because I think the sense ownership is respected in how ownedTagDecls 
are presently handled: at most one ElaboratedType owns a particular TagDecl, 
*and* that type does not seem to be reused elsewhere in the AST.  (This is 
relied on in https://reviews.llvm.org/D131685, which was on my mind when I 
looked at this.)

E.g. consider this expansion of your example:
```
auto e = true ? (struct S*)0 : (true ? (struct S*)0 : (struct S*)0);
```
The first ElaboratedType has an ownedTagDecl; the second is a distinct 
ElaboratedType without an ownedTagDecl, and the third equals the second.  

Now in practice what this means is that `getCommonDecl` when used on binary 
expressions of this sort will always be nullptr, so no harm done.  But suppose 
it is called with X=Y=some ET with an ownedTagDecl (which is the only time I 
believe we could see a non null getCommonDecl result here).  Should the type 
that is returned have that same owned tag decl?  I think that would violate the 
sense of the original type "owning" that decl that seems to be respected 
elsewhere in the AST.

Re type printing, I believe that ownedTagDecls only affects printing of defined 
TagDecls like `struct S { int i; } x;` (which is what I was mostly thinking re 
ownedTagDecls - I don't think we will see those here); for undefined ones like 
`struct S x;`, printing should be unaffected whether there is an ownedTagDecl 
or not.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D128113#3733764 , @mizvekov wrote:

> In D128113#3733051 , @joanahalili 
> wrote:
>
>> We have a translation unit, on which we see an increase of compilation time 
>> and clang memory allocation from 11GB to 14GB. We are working on an isolated 
>> case.
>
> Thanks for looking into this!
>
> What I can imagine may be happening here is that if we instantiate a template 
> with a very large argument pack consisting of mostly the same template 
> arguments, we will create many more `SubstTemplateTypeParmType` nodes with 
> this patch, as they won't unique so much anymore, because each will have a 
> different pack index.

If this is indeed a major problem, and/or if performance is an issue for other 
of the patches in the stack, maybe the solution is along the lines of what I 
raised in 
https://discourse.llvm.org/t/rfc-improving-diagnostics-with-template-specialization-resugaring/64294/5:
 introduce an option that determines how much sugar we construct in the AST, 
and modify `ASTContext::get*Type(...)` accordingly.

For now it could have two possible values, 'medium' and 'maximum', and 
`getSubstTemplateTypeParmType(...)` could be modified so it always uses 
PackIndex=0 whenever it is only set to 'medium'.  And, for later patches, 
resugaring would only be enabled when it is set to 'maximum'.  (And maybe later 
a 'minimum' option could be experimented with which disables all sugar except 
that needed to keep tests passing.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/unittests/AST/ASTContextParentMapTest.cpp:145
+  EXPECT_THAT(Ctx.getParents(FrBLoc), ElementsAre(DynTypedNode::create(FrB)));
+  if (FrATagDecl)
+EXPECT_THAT(Ctx.getParents(*FrATagDecl),

sammccall wrote:
> I'm confused why this is conditional: isn't this the main thing that we're 
> testing? Why do we want the test to silently pass if the AST structure 
> changes so that ownedTagDecl is null? It's hard to tell from reading the code 
> if anything is being tested at all.
Good point, agree we should expect FrATagDecl to be nonnull.  And we should 
check that FrBTagDecl is null, or at a minimum that FrBTagDecl != FrATagDecl, 
because if they are the same decl we are traversing it twice.  (This could 
happen if someone decides to try to reuse the ElaboratedType of A's friend decl 
in B's, to save memory or whatever.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131685

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D131685#3716354 , @balazske wrote:

> I really do not know why parent of the node for the owned `TagDecl` node is 
> the `FriendDecl` and not a `TypeLoc` node, but it is working.
> The code `struct A { friend struct Fr; };` caused no problems either (no 
> duplicate visit of `struct Fr`).

This is because the TraverseDecl(OwnedTagDecl) call is performed in the 
TraverseFriendDecl call, after the TraverseTypeLoc call has already returned -- 
and I assume the ParentMap just looks uses the stack of traversal calls to 
generate the parents.
That said, this does raise the question of whether we should instead be 
changing DEF_TRAVERSE_TYPE(Elaborated) to *always* traverse its owned tag decl; 
that way the parent would be the TypeLoc node.  However in more usual cases an 
ET's owned tag decl is added to the parent DeclContext; e.g. for `struct B {int 
i} data;`,  `B` and `data` are added as separate declarations in the parent 
context (which is annoying -- so probably the Type node really *should* always 
be the parent of that decl, if we were writing the AST from scratch! -- but it 
is what it is).  So if traversed all owned tag decls, we would get some 
duplicate visitations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131685

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131685: [clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Once the FIXME is removed this looks good, but I was involved in this so better 
if @sammccall can give the thumbs up at least to the RecursiveASTVisitor code.




Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1577-1578
 DEF_TRAVERSE_DECL(FriendTemplateDecl, {
+  // FIXME: Traverse getOwnedTagDecl like at the FriendDecl case?
+  // (FriendTemplateDecl is not used)
   if (D->getFriendType())

I don't think anything is necessary here, because we should never see an 
`OwnedTagDecl` here.
In the FriendDecl case the ElaboratedType only has an OwnedTagDecl when `struct 
Fr` has not yet been declared before the friend decl.
In the documentation of FriendTemplateDecl on the other hand these examples are 
given:
```
/// template \ class A {
///   friend class MyVector; // not a friend template
///   template \ friend class B; // not a friend template
///   template \ friend class Foo::Nested; // friend template
/// };
```
So, a FriendTemplateDecl is only created when referencing a nested class 
template member.  But that *must* have been declared before the friend decl, or 
we will get an error:

```
template struct B;
template struct A { template friend struct B::Fr; }; 
//ERROR: no member named 'Fr' in B
```

So the OwnedTagDecl should always be nullptr here/the issue should never arise 
in this case.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131685

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

It was very good to separate this out, thanks.  Can you can do some TMP 
performance testing, to verify the impacts are negligible before taking 
resugaring into consideration, to allay potential concerns?




Comment at: clang/include/clang/AST/Type.h:1838
 /// metaprogramming we'd prefer to keep it as large as possible.
-/// At the moment it has been left as a non-bitfield since this type
-/// safely fits in 64 bits as an unsigned, so there is no reason to
-/// introduce the performance impact of a bitfield.
-unsigned NumArgs;
+unsigned NumArgs : 16;
   };

I can't imagine that limiting template arg index to 16 bits from 32 could be 
all that limiting, but given the comment in the original have you 
tested/confirmed that this is acceptable?



Comment at: clang/include/clang/AST/Type.h:5010
 
-  SubstTemplateTypeParmType(const TemplateTypeParmType *Param, QualType Canon,
-Optional PackIndex)
-  : Type(SubstTemplateTypeParm, Canon, Canon->getDependence()),
-Replaced(Param) {
-SubstTemplateTypeParmTypeBits.PackIndex = PackIndex ? *PackIndex + 1 : 0;
-  }
+  // The templated entity which owned the substituted type parameter.
+  Decl *ReplacedDecl;

This description is inconsistent with the `getReplacedDecl()` documentation: 
this one says its templated, the other says its a template specialization.  I 
think it is a template or templated decl, correct?  In either case, I think you 
can remove this comment and just be sure `getReplacedDecl()` is documented 
accurately.

Also, I wonder if there is a better name than "ReplacedDecl", since that name 
suggests it should return a `TemplateTypeParmDecl` or similar, when its really 
the template-parameterized declaration that holds the parameter this replaces.  
Maybe "ReplacedParent"?  Or "ReplacedParmParent"?



Comment at: clang/include/clang/AST/Type.h:5075
 
-  SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
-QualType Canon,
+  // The template specialization which owned the substituted type parameter.
+  Decl *ReplacedDecl;

See above



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1061
   return false;
+if (Subst1->getReplacedDecl() != Subst2->getReplacedDecl())
+  return false;

Should this line instead be something like `if !IsStructurallyEquivalent(*this, 
Subst1->getReplacedDecl(), Subst2->getReplacedDecl())`?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1073
 const auto *Subst2 = cast(T2);
-if (!IsStructurallyEquivalent(Context,
-  QualType(Subst1->getReplacedParameter(), 0),
-  QualType(Subst2->getReplacedParameter(), 0)))
+if (Subst1->getReplacedDecl() != Subst2->getReplacedDecl())
+  return false;

See above



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1787-1788
   const TemplateTypeParmType *T = TL.getTypePtr();
+  TemplateTypeParmDecl *NewTTPDecl = cast_or_null(
+  TransformDecl(TL.getNameLoc(), T->getDecl()));
+

I don't think this needs to have been moved up here from line 1855; move it 
back down so the comment down there still makes sense


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12767
+Ctx.getQualifiedType(Underlying),
+::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl()));
+  }

This last argument should probably be omitted/nullptr passed, since
# We probably won't encounter any owned tag decls in the types passed to this 
function;
# Even if we did, it would not necessarily be true that they both have non-null 
owned tag decls, and I don't see any nullptr checks in getCommonDecl; and
# Even if we checked for nullptr there, and say passed the same argument to X 
and Y so EX==EY, and that had an owned tag decl, it is not clear to me it would 
be appropriate to construct a new type with the same owned tag decl as another 
type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:12218
+CTN,
+NExpX && NExpY ? Optional(std::min(*NExpX, *NExpY)) : None);
+  }

I'm not clear on how `NExpX` could not equal `NExpY` - could you add a test 
which demonstrates this case?



Comment at: clang/lib/AST/ASTContext.cpp:12804
+  return QualType();
+SmallVector As;
+if (getCommonTemplateArguments(Ctx, As, TX->template_arguments(),

Nit: "As" -> "Args", since that's common elsewhere



Comment at: clang/lib/AST/ASTContext.cpp:12872
   }
 
   SplitQualType SX = X.split(), SY = Y.split();

It would be very helpful to incorporate your description and the description 
from D111283 as comments in this function.  E.g. something like the following 
...



Comment at: clang/lib/AST/ASTContext.cpp:12874
   SplitQualType SX = X.split(), SY = Y.split();
-  if (::removeDifferentTopLevelSugar(SX, SY))
-SX.Ty = ::getCommonType(*this, SX.Ty, SY.Ty).getTypePtr();
-
+  Qualifiers QX, QY;
+  auto Xs = ::unwrapSugar(SX, QX), Ys = ::unwrapSugar(SY, QY);

```
// Desugar SX and SY, setting the sugar and qualifiers aside into Xs and Ys/QX 
and QY, 
// until we reach their underlying "canonical nodes".  (Note these are not 
necessarily 
// canonical types, as their child types may still be sugared.)
```



Comment at: clang/lib/AST/ASTContext.cpp:12876
+  auto Xs = ::unwrapSugar(SX, QX), Ys = ::unwrapSugar(SY, QY);
+  if (SX.Ty != SY.Ty) {
+SX.Ty = ::getCommonNonSugarTypeNode(*this, SX.Ty, SY.Ty).getTypePtr();

```
// The canonical nodes differ. Build a common canonical node out of the two, 
// including any sugar shared by their child types.
```



Comment at: clang/lib/AST/ASTContext.cpp:12878
+SX.Ty = ::getCommonNonSugarTypeNode(*this, SX.Ty, SY.Ty).getTypePtr();
+  } else {
+while (!Xs.empty() && !Ys.empty() && Xs.back().Ty == Ys.back().Ty) {

```
// The canonical nodes were identical: we may have desugared too much.
// Add any common sugar back in.
```



Comment at: clang/lib/AST/ASTContext.cpp:12890
+assert(QX == QY);
+
+  while (!Xs.empty() && !Ys.empty()) {

```
// Even though the remaining sugar nodes in Xs and Ys differ, some may be of 
the same
// type class and have common sugar in their child types.  Walk up these nodes, 
// adding in any such sugar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

> The second paragraph is talking about 'Canonical nodes', not 'Canonical 
> types'.
>
> A canonical node is a type node for which 'isSugared' method returns false.

Thanks for the clarification, but note that that term is not in general use so 
far as I'm aware.   But instead of defining it and getting into details it 
might be clearer, and certainly would have saved me the most time, for the 
description to simply note that this patch introduces 
`ASTContext::getCommonSugaredType(QualType X, QualType Y, bool Unqualified = 
false)`, and puts it to use in type deduction for binary expressions etc., and 
give an example or two to demonstrate.  (More generally on a large patch I 
prefer a description to give me a few starting points, the primary changes 
which necessitate all the others.)

Re the patch itself: it looks good to me other than a few nits, but this has 
such a broad and deep span (intricate details of the AST + intricate details of 
Sema) it is difficult to give it a final thumbs up - really hoping @rsmith 
might take a final look.  But if time runs out it is definitely worth accepting 
as is and seeing how it goes; the benefits exceed the risks.  I will try to 
take a close look at the other patches on your stack, but the same probably 
applies.  You've done a tremendous amount of work here, very impressive.




Comment at: clang/include/clang/AST/ASTContext.h:2807
 
+  FunctionProtoType::ExceptionSpecInfo
+  getCommonExceptionSpec(FunctionProtoType::ExceptionSpecInfo ESI1,

A clearer name might be `combineExceptionSpecs`, or the original 
`mergeExceptionSpecs`, since this is getting the union of their sets of 
exception specs, whereas getCommon* suggests getting the intersection, e.g. 
getCommonSugaredType is getting the intersection of two "sets" of type sugar in 
a sense.  

Also, please add some brief documentation to the function.



Comment at: clang/include/clang/AST/ASTContext.h:2825
+  // the common sugared type between them.
+  void mergeTypeLists(SmallVectorImpl , ArrayRef X,
+  ArrayRef Y);

Any reason this is public?  Or in the header at all?  Seems like it could be a 
static function in the cpp.



Comment at: clang/lib/AST/ASTContext.cpp:12116
+// If we reach the canonical declaration, then Y is older.
+if (DX->isCanonicalDecl())
+  return Y;

I think "canonical" should be replaced with "first" here and 
`isCanonicalDecl()` with `isFirstDecl()`.  So far as I can tell 
`getCanonicalDecl()` returns `getFirstDecl()` everywhere for now, but that 
could conceivably change, and in any case the goal of this code is to find 
which is older, so "first" would be clearer as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

This part of the description is confusing:

> We take two types, X and Y, which we wish to unify as input.
> These types must have the same (qualified or unqualified) canonical
> type.
>
> We dive down fast through top-level type sugar nodes, to the
> underlying canonical node. If these canonical nodes differ, we
> build a common one out of the two, unifying any sugar they had.
> Note that this might involve a recursive call to unify any children
> of those. We then return that canonical node, handling any qualifiers.
>
> If they don't differ, we walk up the list of sugar type nodes we dived
> through, finding the last identical pair, and returning that as the
> result, again handling qualifiers.

The first paragraph says X and Y must have the same canonical type, the second 
suggests they need not.  In all the test cases, they have the same canonical 
type.

IIUC the second paragraph only applies to the stuff done in D130308 
; is that correct?

If so, the description should clearly state this patch only handles the case 
when the canonical types of X and Y are identical, and note that if the 
canonical types of X and Y differ, this patch does nothing, but D130308 
 adds some additional logic to search for 
common sugar in child type nodes of X and Y to use in the merged type.

And of course please add a full description to D130308 
 as well when you have a chance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127695: WIP: clang: Implement Template Specialization Resugaring

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/test/Sema/Resugar/resugar-expr.cpp:244
+// N-error@-2 {{with an rvalue of type 'int'}}
+} // namespace t21

Compositions of MemberExprs/CXXMemberCallExprs have an issue:
```
template  struct A {
  struct Inner {
A1 m;
A1 f();
  } inner;
  Inner g();
};
Z x1 = A().inner.m; //No resugar
Z x2 = A().inner.f(); //No resugar
Z x3 = A().g().m; //No resugar
Z x4 = A().g().f(); //No resugar
Z x5 = A::Inner().m; //ok
```

Composed `CallExprs` seem to work but probably warrant a test, e.g.
```
template  B1 h(B1);
Z x6 = h(Int());
Z x7 = h(h(Int()));
```

https://godbolt.org/z/cszrsvh8d



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127695

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

[David Rector via Phabricator via cfe-commits]

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

This corrects a genuine deficiency in the AST, and the patch LGTM.  Can we 
knock this off Matheus' stack?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128113

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/Expr.cpp:1949-1950
 
 if (E->getCastKind() == CK_ConstructorConversion)
   return cast(SubExpr)->getConstructor();
 

I think the errors prove we should fall back to the most conservative possible 
fix: remove all the other changes and just change these 2 lines to:
```
if (E->getCastKind() == CK_ConstructorConversion) {
  if (auto *CE = dyn_cast(SubExpr)
SubExpr = CE->getSubExpr();
  return cast(SubExpr)->getConstructor();
}
```
I'm can't quite remember but I believe that would solve the bug as narrowly as 
possible.  @kimgr can you give it a try if and see if it avoids the errors?
(If it doesn't, I'm stumped...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/AST/Expr.cpp:1946-1947
   for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) {
 SubExpr = skipImplicitTemporary(E->getSubExpr());
+SubExpr = SubExpr->IgnoreImplicit();
 

aaron.ballman wrote:
> `IgnoreImplicit()` calls `IgnoreImplicitSingleStep()` eventually, and that 
> call does the same work as `skipImplicitTemporary()`, so I think the end 
> result here should be the same.
As I look at this a second time, I just realized...calling IgnoreImplicit here 
mean that the loop only ever runs one iteration, since IgnoreImplicit 
presumably skips over ImplicitCastExprs.  While I liked how Kim did revision 
initially because it seemed to handle the constructor conversions similarly to 
their handling of getSubExprAsWritten() above, now I think something different 
is needed here.

Proposed alternative:
Right now skipIimplicitTemporary does what IgnoreImplicit does *except* skip 
over a) ImplicitCastExprs and b) FullExprs (= ConstantExprs or 
ExprWithCleanups).

Kim has identified that we need to skip over at least ConstantExprs at least in 
this case (i.e. the most conservative possible fix would be to skip over 
ConstantExprs just before the cast in line 1950).

But perhaps the better solution, to forestall future bugs, is to skip over 
FullExprs in skipImplicitTemporary, so that it skips over everything 
IgnoreImplicit does except ImplicitCastExprs.  (And, some documentation should 
be added to `skipImplicitTemporary` to that effect, to aid future maintenance.)

I can't see offhand how the other uses of skipImplicitTemporary would be 
negatively affected by additionally skipping over FullExprs.

Aaron what do you think?  Kim can you verify this alternative would also solve 
the problem without breaking any tests?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

[David Rector via Phabricator via cfe-commits]

davrec added a reviewer: rsmith.
davrec added a subscriber: rsmith.
davrec added a comment.

In D117390#3246799 , @kimgr wrote:

> @daverec I don't have commit access, could you help me land this?
>
> I've rebased on `main` without conflicts, and `ninja check-clang` ran 
> successfully after rebase.
>
> Let me know if there's anything else I can do as far as prep work.

@rsmith is the code owner, and would know best about prep, though he's not 
always available - Richard could you spare a glance and point @kimgr to any 
additional steps needed to land this and D117391 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117390

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[David Rector via Phabricator via cfe-commits]

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

Looks good, thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

> There are already two way more sophisticated (forgive me my bias) 
> implementations in Clang that are for checking if two statements or decls are 
> the same.
>
> 1. ODRHash, used in modules to discover ODR violations
> 2. ASTStructuralEquivalenceContext, used in ASTImporter to discover if two 
> AST nodes are the same or not (as a side effect we diagnose ODR violations as 
> well).
>
> It is not the first time, when such a similarity check is needed (see 
> https://reviews.llvm.org/D75041). Of course reusing the before mentioned 
> components would require some architectural changes, but it might be 
> beneficial.

I do not quite see the overlap.  This patch addresses the structural 
equivalence of DeclRefExprs: as the `std::is_same` (or any type trait example) 
demonstrates, two declarations may be the "same" (e.g. they are both 
`std::false_type::value`), but two DeclRefExprs referring to those declarations 
should not necessarily be considered the "same": the qualifier, specifying the 
path that was taken to look them up, can matter to a user.  It's not a matter 
of the sophistication of the similarity check, it's a matter of what we mean by 
similarity.

I do not see DeclRefExprs handled in ODRHash or ASTStructuralEquivalence.  I do 
see NestedNameSpecifiers handled in both, but I don't think the implementation 
quite matches what is needed here (e.g. in ASTStructuralEquivalence check, if 
one NNS is a NamespaceAlias, the other one is assumed to be a NamespaceAlias: 
not what we want).  It's probably not worth the trouble to factor something 
common out of those; though they should certainly be used as a guide to make 
sure no cases have been missed.


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

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
 /// Determines whether two statement trees are identical regarding

davrec wrote:
> I would strongly support turning these functions into methods of DeclRefExpr 
> and NestedNameSpecifier, not just to avoid the code duplication here but 
> because they sure seem to be of general utility to AST users.
> 
> You just need to clearly motivate these methods in the documentation, which 
> your description for this patch largely does, e.g. something like.:
> 
> ```
> class DeclRefExpr : ... {
>   ...
>   /// Returns whether this \c DeclRefExpr 
>   ///   a. refers to the same canonical declaration as \c other and
>   ///   b. if it has a qualifier, that qualifier refers to the same canonical 
>   ///   declarations as those of \c other .
>   ///
>   /// Consider these two DeclRefExprs:
>   /// \code
>   ///   std::is_same::value
>   ///   std::is_same::value;
>   /// \endcode
>   ///
>   /// We can see that the value static members are different, but in fact
>   /// the \c getDecl() of these two returns the same VarDecl. (...etc)
>   bool isSemanticallyEquivalentTo(const DeclRefExpr *Other) { ... }
> };
> ...
> class NestedNameSpecifier {
>   ...
>   /// Returns whether this refers to the same sequence of 
> identifiers/canonical declarations
>   /// as \c Other.  (Then I would repeat the std::is_same example here, since 
> that
>   /// really makes the issue clear.  And also describe what this returns when 
> other is nullptr or
>   /// when getPrefix() is nullptr while other->getPrefix() is non-null -- 
> maybe add a parameter if
>   /// its not clear in general what the behavior should be.)
>   bool isSemanticallyEquivalentTo(const NestedNameSpecifier *Other, bool 
> RecurseToPrefix = true) { ... }
> };
> 
> ```
Also if doing this it might be nice to add `isSyntacticallyEquivalentTo(other)` 
methods alongside the semantic versions, defined the same except they don't 
desugar the type nor get the underlying NamespaceDecl from a 
NamespaceAliasDecl.  This would be simple and, given that you found that two 
NestedNameSpecifiers representing the exact same syntax are nonetheless 
different pointers, it could be very useful for AST users concerned with syntax 
rather than semantics.

(Btw note the DeclRefExpr's syntactic method should still use 
getDecl()->getCanonicalDecl(), since, confusingly, getCanonicalDecl simply 
fetches the FirstDecl from the various redeclarations, rather than doing any 
kind of desugaring as getCanonicalType does.  And fortunately I don't think it 
is necessary to manually "desugar" the canonical decl in the semantic version, 
since a DeclRefExpr's getDecl is always a ValueDecl, which I don't think can 
ever be "sugar" for some underlying decl, as say a NamespaceAliasDecl is for a 
NamespaceDecl -- so that part should be fine.)



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

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.

steakhal wrote:
> davrec wrote:
> > It occurs to me this needs to be recursive, to check a sequence of 
> > qualifiers, i.e.
> > ```
> >   const Type *LTy = Left->getAsType();
> >   const Type *RTy = Right->getAsType();
> >   if (!LTy || !RTy) 
> > return true;
> >   if (LTy->getCanonicalTypeUnqualified() !=
> >RTy->getCanonicalTypeUnqualified())
> > return false;
> >   return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
> > ```
> > 
> > The reason is, if there is a prefix qualifier to this qualifier, we run 
> > into the original problem, e.g.:
> > ```
> > struct Base {
> >   struct Inner { 
> > static const bool value = true;
> >   };
> > };
> > struct Outer1 : Base {};
> > struct Outer2 : Base {};
> > 
> > // We do not want the following to warn, but without checking the prefix of 
> > Inner, 
> > // I believe these will be interpreted as equivalent DeclRefs and will warn:
> > auto sink = Outer1::Inner::value || Outer2::Inner::value;
> > ```
> > 
> You are right, but that would require me to implement all the possible kinds 
> of NNS, in addition to the types.
> And it's not entirely clear to me (yet) what the desired behavior should be 
> for implementing this.
I agree it's not entirely clear what should be the desired behavior in that 
rare case; I suppose it might be appropriate to introduce a `bool 
RecurseToPrefix` param that determines whether you do the while loop or finish 
after one iteration, mostly so that others using this function can ponder the 
decision for themselves.



Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
 /// Determines whether two statement trees are identical regarding

I would strongly support turning these functions into methods of DeclRefExpr 
and NestedNameSpecifier, not just to avoid the code duplication here but 
because they sure seem to be of general utility to AST users.

You just need to clearly motivate these methods in the documentation, which 
your description for this patch largely does, e.g. something like.:

```
class DeclRefExpr : ... {
  ...
  /// Returns whether this \c DeclRefExpr 
  ///   a. refers to the same canonical declaration as \c other and
  ///   b. if it has a qualifier, that qualifier refers to the same canonical 
  ///   declarations as those of \c other .
  ///
  /// Consider these two DeclRefExprs:
  /// \code
  ///   std::is_same::value
  ///   std::is_same::value;
  /// \endcode
  ///
  /// We can see that the value static members are different, but in fact
  /// the \c getDecl() of these two returns the same VarDecl. (...etc)
  bool isSemanticallyEquivalentTo(const DeclRefExpr *Other) { ... }
};
...
class NestedNameSpecifier {
  ...
  /// Returns whether this refers to the same sequence of identifiers/canonical 
declarations
  /// as \c Other.  (Then I would repeat the std::is_same example here, since 
that
  /// really makes the issue clear.  And also describe what this returns when 
other is nullptr or
  /// when getPrefix() is nullptr while other->getPrefix() is non-null -- maybe 
add a parameter if
  /// its not clear in general what the behavior should be.)
  bool isSemanticallyEquivalentTo(const NestedNameSpecifier *Other, bool 
RecurseToPrefix = true) { ... }
};

```


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

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

`throughUsingDecl` seems like a good solution, though I defer to regular 
ASTMatchers users.

One other thought: can we add diagnostic notes using this new information, e.g.

  struct A1 {
  protected:
using IntType = char;
  };
  
  struct A2 {
  protected:
using IntType = unsigned long;
  };
  
  struct B : A1, A2 {
using A1::IntType;
  };
  
  struct C : B {
IntType foo;
void setFoo(IntType V) {
  foo = V;
}
  };
  
  
  int main() {
D d;
d.setFoo((unsigned long)-1); // warning: implicit conversion loses info 
  }

It would be nice to add a note pointing to `using A1::IntType` saying 
"introduced here".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114251

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

A couple thoughts/cases to consider...




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:52
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {

This function should probably either be made into a lambda private to 
`areEquivalentDeclRefs()`, or renamed to make clear it does not apply 
generically to all NNS's.  

Part of the reason is that this is only implemented for type NNS's.  But the 
more important reason is that, when either a) `!Left || !Right`, or b) 
`!Left->getAsType() || !Right->getAsType()`, this returns true, since 
(presumably*) this gives us the desired behavior within 
areEquivalentDeclRefs(), despite that in general a null NNS should probably not 
be considered the same as a nonnull NNS.  

(*Is this indeed the desired behavior? Probably should add some tests that 
check qualified DeclRefExprs against unqualified DeclRefExprs, to be sure.)



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:53
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {
+  const Type *LTy = Left->getAsType();

Suggest you move the null check from `areEquivalentDeclRefs()` here, i.e.
```
if (!Left || !Right)
  return true;
```
mainly since this is needs to be done in recursive calls (see below), but also 
since you do the same logic on LTy and RTy subsequently.





Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.

It occurs to me this needs to be recursive, to check a sequence of qualifiers, 
i.e.
```
  const Type *LTy = Left->getAsType();
  const Type *RTy = Right->getAsType();
  if (!LTy || !RTy) 
return true;
  if (LTy->getCanonicalTypeUnqualified() !=
   RTy->getCanonicalTypeUnqualified())
return false;
  return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
```

The reason is, if there is a prefix qualifier to this qualifier, we run into 
the original problem, e.g.:
```
struct Base {
  struct Inner { 
static const bool value = true;
  };
};
struct Outer1 : Base {};
struct Outer2 : Base {};

// We do not want the following to warn, but without checking the prefix of 
Inner, 
// I believe these will be interpreted as equivalent DeclRefs and will warn:
auto sink = Outer1::Inner::value || Outer2::Inner::value;
```




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:72
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+return true;

Suggest you move this null check into areEquivalentNameSpecifier, see above


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

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

[David Rector via Phabricator via cfe-commits]

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

Looks great, thanks for identifying the need and banging this out so quickly.
Hope you had some time to enjoy the holiday with your family!
Dave




Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2106
+TRY_TO(TraverseDecl(Child));
+}
   }

So the idea is, the UsingDecl which introduced the shadows will have already 
been traversed within its DeclStmt via the body traversal, but not the 
UsingShadowDecls it introduced?  Nicely spotted.



Comment at: clang/include/clang/AST/Type.h:4381
+public:
+  NamedDecl *getFoundDecl() const { return Found; }
+  QualType getUnderlyingType() const { return UnderlyingTy; }

sammccall wrote:
> davrec wrote:
> >  I would rename this to `getDecl()`, to match the interface for other types 
> > that just wrap a decl.  E.g. if something is a RecordType I know I can call 
> > getDecl() to get the RecordDecl; likewise a TypedefType::getDecl() returns 
> > a TypedefNameDecl; I think it would follow this pattern for UsingType to 
> > have a getDecl() method that returns a UsingShadowDecl (or whatever else it 
> > can be, per other question).
> > 
> I do prefer `getFoundDecl()` for a few reasons:
>  - the parallel with `NamedDecl::getFoundDecl()` is closer/more important 
> than with `TypedefDecl` I think
>  - there are always two decls here: the invisible UsingShadowDecl and the 
> underlying one. Saying "decl" without a hint seems error-prone to me. 
> (Compare with TypedefType where in general there's no underlying decl).
>  - I **do** find TypedefType::getDecl() confusing, because wherever I see it 
> called I have to verify that it's TypedefType::getDecl() rather than some 
> Type::getDecl() to be sure I understand the semantics.
> 
> Would be happy to hear a third opinion here though.
I'm persuaded by your reasoning, particularly that UsingShadowDecl is invisible 
and thus returning one already introduces some confusion -- i.e. a user might 
reasonably expect getDecl() to return a UsingDecl, so better to call this 
something with other parallels.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114251

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114251: [AST] Add a sugar type for types found via UsingDecl

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Looks good, a few notes.




Comment at: clang/include/clang/AST/ASTContext.h:1559
 
+  QualType getUsingType(const NamedDecl *Found, QualType Underlying) const;
+

I believe you mentioned ObjC considerations might require expanding beyond 
UsingShadowDecl as the type of `Found` -- is that why this is a generic 
NamedDecl?  I.e. can `Found` indeed be other things than a UsingShadowDecl?



Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:984
 
+DEF_TRAVERSE_TYPE(UsingType, { TRY_TO(TraverseType(T->getUnderlyingType())); })
 DEF_TRAVERSE_TYPE(UnresolvedUsingType, {})

This should just be DEF_TRAVERSE_TYPE(UsingType, {}), i.e. same as for 
TypedefType -- RecursiveASTVisitor only visits the child nodes, to avoid 
retraversal of any nodes; by traversing the desugared node this would jump back 
to a node it probably already traversed.



Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1256
 
+DEF_TRAVERSE_TYPELOC(UsingType,
+ { TRY_TO(TraverseTypeLoc(TL.getUnderlyingLoc())); })

DEF_TRAVERSE_TYPELOC(UsingType, {})



Comment at: clang/include/clang/AST/Type.h:4381
+public:
+  NamedDecl *getFoundDecl() const { return Found; }
+  QualType getUnderlyingType() const { return UnderlyingTy; }

 I would rename this to `getDecl()`, to match the interface for other types 
that just wrap a decl.  E.g. if something is a RecordType I know I can call 
getDecl() to get the RecordDecl; likewise a TypedefType::getDecl() returns a 
TypedefNameDecl; I think it would follow this pattern for UsingType to have a 
getDecl() method that returns a UsingShadowDecl (or whatever else it can be, 
per other question).




Comment at: clang/include/clang/AST/TypeLoc.h:671
+/// Wrapper for source info for types used via transparent aliases.
+class UsingTypeLoc : public ConcreteTypeLoc {

Should this be analogous to TypedefTypeLoc, with base as
```
 InheritingConcreteTypeLoc
```
?



Comment at: clang/include/clang/AST/TypeProperties.td:366
+let Class = UsingType in {
+  def : Property<"foundDeclaration", NamedDeclRef> {
+let Read = [{ node->getFoundDecl() }];

Rename to "declaration" iff renaming UsingType::getFoundDecl to 
UsingType::getDecl



Comment at: clang/lib/AST/ASTContext.cpp:4612
+Canon = getCanonicalType(Underlying);
+  UsingType *NewType = new UsingType(Found, Underlying, Canon);
+  Types.push_back(NewType);

UsingType *NewType = new (*this, TypeAlignment) UsingType(Found, Underlying, 
Canon)



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:952
+  return false;
+if (!IsStructurallyEquivalent(Context,
+  cast(T1)->getUnderlyingType(),

Is there a good reason this checks the underlying type in addition to 
getFoundDecl()? I'm looking at the Typedef case below and it only checks the 
decl, does this need to be different from that?



Comment at: clang/lib/AST/Type.cpp:1819
 
+Type *VisitUsingType(const UsingType *T) {
+  return Visit(T->getUnderlyingType());

Do we need this here?  I ask because I mainly because don't see a 
VisitTypedefType, not 100% sure what this class does though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114251

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104182: [clang][NFC] Add IsAnyDestructorNoReturn field to CXXRecord instead of calculating it on demand

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

Was this performance hit when using the static analyzer?  A quick search 
suggests `isAnyDestructorNoReturn()` is only called within the analyzer, 
whereas comparable CXXRecordDecl methods whose results are stored 
(`hasIrrelevantDestructor()` etc.) seem to be called somewhere by Sema.

So non-users of the analyzer would not benefit from this change, and will incur 
a slight cost, IIUC.  Is that cost remotely noticeable?  Probably not, but a 
quick test along those lines would be helpful.

All in all this is probably good and advisable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104182

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101777: [clang] p1099 1/5: [NFC] Break out BaseUsingDecl from UsingDecl

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

Looks good, thanks for doing this!


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

https://reviews.llvm.org/D101777

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102241: [clang] p1099 4/5: using enum EnumTag

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:520-523
+  /// Like InstantiatedFromUsingDecl, but for using-enum declarations. Maps
+  /// from the instantiated using-enum to the templated decl from whence it
+  /// came.
+  llvm::DenseMap InstantiatedFromUsingEnumDecl;

davrec wrote:
> We need a detailed example in the documentation, since IIUC P1099 does not 
> allow a using-enum-declaration to name "dependent" enums and thus is 
> distinguished from using-declarations. Specifically the wording is:
> > using-enum-declaration:
> >  using elaborated-enum-specifier ;
> > 1. The elaborated-enum-specifier shall not name a dependent type and the 
> > type shall have a reachable enum-specifier.
> > ...
> (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html)
> 
> Now I'm not 100% clear on what that wording permits, but we need an example 
> of how a UsingEnumDecl can be an instantiation.  Something like this maybe?  
> ```
> template
> struct Foo {
>   enum E { e = N };
> };
> template
> struct Bar : Foo {
>   using enum Foo::E; //Allowed per P1099?
> };
> Bar<1>;
> ```
> 
> We can also clarify the types here to
> ```
> llvm::DenseMap
> ```
> since there are no UnresolvedUsing*Decl` versions to account for, as there 
> were with using decls.
It occurs to me that, duh, non-dependent declarations in a template still need 
to be instantiated.
So in summary I would just change these lines to this:
```
  /// Like InstantiatedFromUsingDecl, but for using-enum-declarations. Maps
  /// from the instantiated using-enum to the templated decl from whence it
  /// came.
  /// Note that using-enum-declarations cannot be dependent and
  /// thus will never be instantiated from an "unresolved"
  /// version thereof (as with using-declarations), so each mapping is from
  /// a (resolved) UsingEnumDecl to a (resolved) UsingEnumDecl.
  llvm::DenseMap 
InstantiatedFromUsingEnumDecl;  
```


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

https://reviews.llvm.org/D102241

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102241: [clang] p1099 4/5: using enum EnumTag

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:520-523
+  /// Like InstantiatedFromUsingDecl, but for using-enum declarations. Maps
+  /// from the instantiated using-enum to the templated decl from whence it
+  /// came.
+  llvm::DenseMap InstantiatedFromUsingEnumDecl;

We need a detailed example in the documentation, since IIUC P1099 does not 
allow a using-enum-declaration to name "dependent" enums and thus is 
distinguished from using-declarations. Specifically the wording is:
> using-enum-declaration:
>  using elaborated-enum-specifier ;
> 1. The elaborated-enum-specifier shall not name a dependent type and the type 
> shall have a reachable enum-specifier.
> ...
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html)

Now I'm not 100% clear on what that wording permits, but we need an example of 
how a UsingEnumDecl can be an instantiation.  Something like this maybe?  
```
template
struct Foo {
  enum E { e = N };
};
template
struct Bar : Foo {
  using enum Foo::E; //Allowed per P1099?
};
Bar<1>;
```

We can also clarify the types here to
```
llvm::DenseMap
```
since there are no UnresolvedUsing*Decl` versions to account for, as there were 
with using decls.



Comment at: clang/include/clang/AST/ASTContext.h:906-907
 
   /// If the given using decl \p Inst is an instantiation of a
   /// (possibly unresolved) using decl from a template instantiation,
   /// return it.

```
  /// If the given using decl \p Inst is an instantiation of
  /// another (possibly unresolved) using decl, return it.
```



Comment at: clang/include/clang/AST/ASTContext.h:915-918
+  /// If the given using-enum decl \p Inst is an instantiation of a
+  /// (possibly unresolved) using decl from a template instantiation,
+  /// return it.
+  NamedDecl *getInstantiatedFromUsingEnumDecl(NamedDecl *Inst);

```
  /// If the given using-enum decl \p Inst is an instantiation of
  /// another using-enum decl, return it.
  UsingEnumDecl *getInstantiatedFromUsingEnumDecl(UsingEnumDecl *Inst);
```



Comment at: clang/include/clang/AST/ASTContext.h:922
+  /// of the using enum decl \p Pattern of a class template.
+  void setInstantiatedFromUsingEnumDecl(NamedDecl *Inst, NamedDecl *Pattern);
+

`UsingEnumDecl *Inst, UsingEnumDecl *Pattern`



Comment at: clang/lib/AST/ASTContext.cpp:1574
 
+NamedDecl *ASTContext::getInstantiatedFromUsingEnumDecl(NamedDecl *UUD) {
+  auto Pos = InstantiatedFromUsingEnumDecl.find(UUD);

NamedDecl -> UsingEnumDecl



Comment at: clang/lib/AST/ASTContext.cpp:1582-1583
+
+void ASTContext::setInstantiatedFromUsingEnumDecl(NamedDecl *Inst,
+  NamedDecl *Pattern) {
+  assert(isa(Pattern) &&

NamedDecl -> UsingEnumDecl


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

https://reviews.llvm.org/D102241

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101777: [clang] p1099 1/5: [NFC] Break out BaseUsingDecl from UsingDecl

[David Rector via Phabricator via cfe-commits]

davrec accepted this revision.
davrec added a comment.

Sorry for the delay.
Richard should probably weigh in before absolutely committing to

1. BaseUsingDecl/UsingEnumDecl/UsingDecl (as implemented here) vs. single 
UsingDecl (Nathan's original approach) and
2. Renaming `getUsingDecl()` to `getIntroducer()` (if it is to return a 
`BaseUsingDecl`).

(Main rationale for separating UsingEnumDecl vs. UsingDecl: parallels the 
naming distinction in P1099  between a 
"using-declaration" and a "using-enum-declaration".  Cons: churn, adds to quite 
a collection of Using*-named AST nodes already.  Nonetheless I favor 
distinguishing them.)




Comment at: clang/include/clang/AST/DeclCXX.h:3390
   ConstructorUsingShadowDecl(ASTContext , DeclContext *DC, SourceLocation 
Loc,
- UsingDecl *Using, NamedDecl *Target,
+ BaseUsingDecl *Using, NamedDecl *Target,
  bool TargetInVirtualBase)

`BaseUsingDecl` here should remain `UsingDecl`.
Additionally, I would add a `UsingDecl *getIntroducer() { return 
cast(UsingShadowDecl::getIntroducer()); }` method here.



Comment at: clang/lib/AST/DeclCXX.cpp:3026
 CXXRecordDecl *ConstructorUsingShadowDecl::getNominatedBaseClass() const {
-  return getUsingDecl()->getQualifier()->getAsRecordDecl();
+  return cast(getIntroducer())->getQualifier()->getAsRecordDecl();
 }

With the additional `ConstructorUsingShadowDecl::getIntroducer()` method 
suggested elsewhere, the cast won't be necessary here.


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

https://reviews.llvm.org/D101777

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits