[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG6a5e08cc4a5c: [AST] injected-class-name is not a redecl, 
even in template specializations (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D112765?vs=383351=384065#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

Files:
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/lib/AST/ASTDumper.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-decl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp

Index: clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
===
--- clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
+++ clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
@@ -69,7 +69,7 @@
 // CHECK-NEXT: | | | `-Destructor simple irrelevant trivial
 // CHECK-NEXT: | | |-TemplateArgument type 'int'
 // CHECK-NEXT: | | | `-BuiltinType [[ADDR_9:0x[a-z0-9]*]] 'int'
-// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_10:0x[a-z0-9]*]] prev [[ADDR_8]]  col:30 implicit struct S
+// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_10:0x[a-z0-9]*]]  col:30 implicit struct S
 // CHECK-NEXT: | | |-CXXConstructorDecl [[ADDR_11:0x[a-z0-9]*]]  col:3 used S 'void (int, int *)'
 // CHECK-NEXT: | | | |-ParmVarDecl [[ADDR_12:0x[a-z0-9]*]]  col:8 'int'
 // CHECK-NEXT: | | | |-ParmVarDecl [[ADDR_13:0x[a-z0-9]*]]  col:13 'int *'
Index: clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
===
--- clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
+++ clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
@@ -120,7 +120,7 @@
 // CHECK-NEXT: | | |-TemplateArgument type 'float &'
 // CHECK-NEXT: | | | `-LValueReferenceType [[ADDR_7:0x[a-z0-9]*]] 'float &'
 // CHECK-NEXT: | | |   `-BuiltinType [[ADDR_8:0x[a-z0-9]*]] 'float'
-// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_9:0x[a-z0-9]*]] prev [[ADDR_6]]  col:29 implicit struct remove_reference
+// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_9:0x[a-z0-9]*]]  col:29 implicit struct remove_reference
 // CHECK-NEXT: | | `-TypedefDecl [[ADDR_10:0x[a-z0-9]*]]  col:67 referenced type 'float':'float'
 // CHECK-NEXT: | |   `-SubstTemplateTypeParmType [[ADDR_11:0x[a-z0-9]*]] 'float' sugar
 // CHECK-NEXT: | | |-TemplateTypeParmType [[ADDR_12:0x[a-z0-9]*]] '_Tp' dependent depth 0 index 0
@@ -137,7 +137,7 @@
 // CHECK-NEXT: |   |-TemplateArgument type 'short &'
 // CHECK-NEXT: |   | `-LValueReferenceType [[ADDR_15:0x[a-z0-9]*]] 'short &'
 // CHECK-NEXT: |   |   `-BuiltinType [[ADDR_16:0x[a-z0-9]*]] 'short'
-// CHECK-NEXT: |   |-CXXRecordDecl [[ADDR_17:0x[a-z0-9]*]] prev [[ADDR_14]]  col:29 implicit struct remove_reference
+// CHECK-NEXT: |   |-CXXRecordDecl [[ADDR_17:0x[a-z0-9]*]]  col:29 implicit struct remove_reference
 // CHECK-NEXT: |   `-TypedefDecl [[ADDR_18:0x[a-z0-9]*]]  col:67 referenced type 'short':'short'
 // CHECK-NEXT: | `-SubstTemplateTypeParmType [[ADDR_19:0x[a-z0-9]*]] 'short' sugar
 // CHECK-NEXT: |   |-TemplateTypeParmType [[ADDR_12]] '_Tp' dependent depth 0 index 0
Index: clang/test/AST/ast-dump-decl.cpp
===
--- clang/test/AST/ast-dump-decl.cpp
+++ clang/test/AST/ast-dump-decl.cpp
@@ -321,7 +321,7 @@
 // CHECK-NEXT:  | |-TemplateArgument type 'testClassTemplateDecl::A'
 // CHECK-NEXT:  | | `-RecordType 0{{.+}} 'testClassTemplateDecl::A'
 // CHECK-NEXT:  | |   `-CXXRecord 0x{{.+}} 'A'
-// CHECK-NEXT:  | |-CXXRecordDecl 0x{{.+}} prev 0x{{.+}}  col:30 implicit class TestClassTemplate
+// CHECK-NEXT:  | |-CXXRecordDecl 0x{{.+}}  col:30 implicit class TestClassTemplate
 // CHECK-NEXT:  | |-AccessSpecDecl 0x{{.+}}  col:3 public
 // CHECK-NEXT:  | |-CXXConstructorDecl 0x{{.+}}  col:5 used TestClassTemplate 'void ()'
 // CHECK-NEXT:  | |-CXXDestructorDecl 0x{{.+}}  col:5 used ~TestClassTemplate 'void () noexcept'
@@ -358,7 +358,7 @@
 // CHECK-NEXT:  |-TemplateArgument type 'testClassTemplateDecl::C'
 // CHECK-NEXT:  | `-RecordType 0{{.+}} 'testClassTemplateDecl::C'
 // CHECK-NEXT:  |   `-CXXRecord 0x{{.+}} 'C'
-// CHECK-NEXT:  |-CXXRecordDecl 0x{{.+}} prev 0x{{.+}}  col:30 implicit class TestClassTemplate
+// CHECK-NEXT:  |-CXXRecordDecl 0x{{.+}}  col:30 implicit class TestClassTemplate
 // CHECK-NEXT:  |-AccessSpecDecl 0x{{.+}}  col:3 public
 // CHECK-NEXT:  |-CXXConstructorDecl 0x{{.+}}  col:5 TestClassTemplate 'void ()'
 // CHECK-NEXT:  |-CXXDestructorDecl 0x{{.+}}  col:5 ~TestClassTemplate 'void ()' noexcept-unevaluated 0x{{.+}}
@@ -376,7 +376,7 @@
 // 

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added inline comments.



Comment at: clang/lib/AST/ASTDumper.cpp:94
 auto *Redecl = dyn_cast(RedeclWithBadType);
-if (!Redecl) {
-  // Found the injected-class-name for a class template. This will be 
dumped

hokein wrote:
> `assert(Redecl)`
Changed the dyn_cast to a cast instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

looks good, thanks.




Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1685
-  // We don't want to visit injected-class-names in this traversal.
-  if (cast(RD)->isInjectedClassName())
-continue;

I would add a  assertion here: 
`assert(!cast(RD)->isInjectedClassName())`.



Comment at: clang/lib/AST/ASTDumper.cpp:94
 auto *Redecl = dyn_cast(RedeclWithBadType);
-if (!Redecl) {
-  // Found the injected-class-name for a class template. This will be 
dumped

`assert(Redecl)`



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1910
 
+  if (IsInjectedClassName)
+assert(Record->isInjectedClassName() && "Broken injected-class-name");

sammccall wrote:
> hokein wrote:
> > it is unclear to me what's the intention of the assertion.
> We needed to do a few weird things (and to assume some things about the 
> input) to initialize the injected-class-name, it verifies we got them all 
> right.
> 
> (This assertion is also in ActOnStartCXXMemberDeclarations())
ok, I see. didn't know this is derived from `ActOnStartCXXMemberDeclarations`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Aaron Ballman via Phabricator via cfe-commits]

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

Aside from the cast request, this LGTM, thank you!




Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1841-1842
+   /*DelayTypeCreation=*/IsInjectedClassName);
+  if (IsInjectedClassName)
+SemaRef.Context.getTypeDeclType(Record, cast(Owner));
 

sammccall wrote:
> hokein wrote:
> > aaron.ballman wrote:
> > > Why is this call needed? (It seems strange to me that we call it and 
> > > ignore the return value.)
> > my understanding is
> >   - `getTypeDeclType` is more than a getter, it also has a side-effort -- 
> > if the type of the passed `Record` is empty, it creates a type, and 
> > propagates the type to `Record->TypeForDecl`;
> >   - from the above line, we delay the type creation when 
> > `IsInjectedClassName` is true;
> >   - so we need to create a type for the `Record` by invoking 
> > `getTypeDeclType`;
> > 
> > might be worth a comment.
> > 
> That's exactly right. The injected-class-name is an independent decl but 
> yields the same RecordType, and getTypeDeclType has a hacky interface so we 
> can force this link.
> (It wasn't needed in the old version of the code, because when it was a 
> redecl getTypeDeclType would yield the same type in any case)
> 
> I should have mentioned this is all derived from the injected-class-name 
> handling in Sema::ActOnStartCXXMemberDeclarations(). It lacks comments, but 
> I'll add a few while here.
Ah, thank you for the explanation! I think a `(void)` on the result will also 
help make it clear that we intentionally are ignoring that result despite it 
looking like a getter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1841-1842
+   /*DelayTypeCreation=*/IsInjectedClassName);
+  if (IsInjectedClassName)
+SemaRef.Context.getTypeDeclType(Record, cast(Owner));
 

hokein wrote:
> aaron.ballman wrote:
> > Why is this call needed? (It seems strange to me that we call it and ignore 
> > the return value.)
> my understanding is
>   - `getTypeDeclType` is more than a getter, it also has a side-effort -- if 
> the type of the passed `Record` is empty, it creates a type, and propagates 
> the type to `Record->TypeForDecl`;
>   - from the above line, we delay the type creation when 
> `IsInjectedClassName` is true;
>   - so we need to create a type for the `Record` by invoking 
> `getTypeDeclType`;
> 
> might be worth a comment.
> 
That's exactly right. The injected-class-name is an independent decl but yields 
the same RecordType, and getTypeDeclType has a hacky interface so we can force 
this link.
(It wasn't needed in the old version of the code, because when it was a redecl 
getTypeDeclType would yield the same type in any case)

I should have mentioned this is all derived from the injected-class-name 
handling in Sema::ActOnStartCXXMemberDeclarations(). It lacks comments, but 
I'll add a few while here.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1910
 
+  if (IsInjectedClassName)
+assert(Record->isInjectedClassName() && "Broken injected-class-name");

hokein wrote:
> it is unclear to me what's the intention of the assertion.
We needed to do a few weird things (and to assume some things about the input) 
to initialize the injected-class-name, it verifies we got them all right.

(This assertion is also in ActOnStartCXXMemberDeclarations())


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 383351.
sammccall added a comment.

Add a comment, remove some unneccesary special cases from traversals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

Files:
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/lib/AST/ASTDumper.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-decl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp

Index: clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
===
--- clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
+++ clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
@@ -69,7 +69,7 @@
 // CHECK-NEXT: | | | `-Destructor simple irrelevant trivial
 // CHECK-NEXT: | | |-TemplateArgument type 'int'
 // CHECK-NEXT: | | | `-BuiltinType [[ADDR_9:0x[a-z0-9]*]] 'int'
-// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_10:0x[a-z0-9]*]] prev [[ADDR_8]]  col:30 implicit struct S
+// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_10:0x[a-z0-9]*]]  col:30 implicit struct S
 // CHECK-NEXT: | | |-CXXConstructorDecl [[ADDR_11:0x[a-z0-9]*]]  col:3 used S 'void (int, int *)'
 // CHECK-NEXT: | | | |-ParmVarDecl [[ADDR_12:0x[a-z0-9]*]]  col:8 'int'
 // CHECK-NEXT: | | | |-ParmVarDecl [[ADDR_13:0x[a-z0-9]*]]  col:13 'int *'
Index: clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
===
--- clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
+++ clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
@@ -120,7 +120,7 @@
 // CHECK-NEXT: | | |-TemplateArgument type 'float &'
 // CHECK-NEXT: | | | `-LValueReferenceType [[ADDR_7:0x[a-z0-9]*]] 'float &'
 // CHECK-NEXT: | | |   `-BuiltinType [[ADDR_8:0x[a-z0-9]*]] 'float'
-// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_9:0x[a-z0-9]*]] prev [[ADDR_6]]  col:29 implicit struct remove_reference
+// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_9:0x[a-z0-9]*]]  col:29 implicit struct remove_reference
 // CHECK-NEXT: | | `-TypedefDecl [[ADDR_10:0x[a-z0-9]*]]  col:67 referenced type 'float':'float'
 // CHECK-NEXT: | |   `-SubstTemplateTypeParmType [[ADDR_11:0x[a-z0-9]*]] 'float' sugar
 // CHECK-NEXT: | | |-TemplateTypeParmType [[ADDR_12:0x[a-z0-9]*]] '_Tp' dependent depth 0 index 0
@@ -137,7 +137,7 @@
 // CHECK-NEXT: |   |-TemplateArgument type 'short &'
 // CHECK-NEXT: |   | `-LValueReferenceType [[ADDR_15:0x[a-z0-9]*]] 'short &'
 // CHECK-NEXT: |   |   `-BuiltinType [[ADDR_16:0x[a-z0-9]*]] 'short'
-// CHECK-NEXT: |   |-CXXRecordDecl [[ADDR_17:0x[a-z0-9]*]] prev [[ADDR_14]]  col:29 implicit struct remove_reference
+// CHECK-NEXT: |   |-CXXRecordDecl [[ADDR_17:0x[a-z0-9]*]]  col:29 implicit struct remove_reference
 // CHECK-NEXT: |   `-TypedefDecl [[ADDR_18:0x[a-z0-9]*]]  col:67 referenced type 'short':'short'
 // CHECK-NEXT: | `-SubstTemplateTypeParmType [[ADDR_19:0x[a-z0-9]*]] 'short' sugar
 // CHECK-NEXT: |   |-TemplateTypeParmType [[ADDR_12]] '_Tp' dependent depth 0 index 0
Index: clang/test/AST/ast-dump-decl.cpp
===
--- clang/test/AST/ast-dump-decl.cpp
+++ clang/test/AST/ast-dump-decl.cpp
@@ -321,7 +321,7 @@
 // CHECK-NEXT:  | |-TemplateArgument type 'testClassTemplateDecl::A'
 // CHECK-NEXT:  | | `-RecordType 0{{.+}} 'testClassTemplateDecl::A'
 // CHECK-NEXT:  | |   `-CXXRecord 0x{{.+}} 'A'
-// CHECK-NEXT:  | |-CXXRecordDecl 0x{{.+}} prev 0x{{.+}}  col:30 implicit class TestClassTemplate
+// CHECK-NEXT:  | |-CXXRecordDecl 0x{{.+}}  col:30 implicit class TestClassTemplate
 // CHECK-NEXT:  | |-AccessSpecDecl 0x{{.+}}  col:3 public
 // CHECK-NEXT:  | |-CXXConstructorDecl 0x{{.+}}  col:5 used TestClassTemplate 'void ()'
 // CHECK-NEXT:  | |-CXXDestructorDecl 0x{{.+}}  col:5 used ~TestClassTemplate 'void () noexcept'
@@ -358,7 +358,7 @@
 // CHECK-NEXT:  |-TemplateArgument type 'testClassTemplateDecl::C'
 // CHECK-NEXT:  | `-RecordType 0{{.+}} 'testClassTemplateDecl::C'
 // CHECK-NEXT:  |   `-CXXRecord 0x{{.+}} 'C'
-// CHECK-NEXT:  |-CXXRecordDecl 0x{{.+}} prev 0x{{.+}}  col:30 implicit class TestClassTemplate
+// CHECK-NEXT:  |-CXXRecordDecl 0x{{.+}}  col:30 implicit class TestClassTemplate
 // CHECK-NEXT:  |-AccessSpecDecl 0x{{.+}}  col:3 public
 // CHECK-NEXT:  |-CXXConstructorDecl 0x{{.+}}  col:5 TestClassTemplate 'void ()'
 // CHECK-NEXT:  |-CXXDestructorDecl 0x{{.+}}  col:5 ~TestClassTemplate 'void ()' noexcept-unevaluated 0x{{.+}}
@@ -376,7 +376,7 @@
 // CHECK-NEXT:  |-TemplateArgument type 'testClassTemplateDecl::D'
 // CHECK-NEXT:  | `-RecordType 0{{.+}} 'testClassTemplateDecl::D'
 // CHECK-NEXT:  |   `-CXXRecord 0x{{.+}} 'D'
-// 

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

looks like 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L1684-L1686
 can be simplified as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I think we also want to update the ast dumper bit: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTDumper.cpp#L94-L100




Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1841-1842
+   /*DelayTypeCreation=*/IsInjectedClassName);
+  if (IsInjectedClassName)
+SemaRef.Context.getTypeDeclType(Record, cast(Owner));
 

aaron.ballman wrote:
> Why is this call needed? (It seems strange to me that we call it and ignore 
> the return value.)
my understanding is
  - `getTypeDeclType` is more than a getter, it also has a side-effort -- if 
the type of the passed `Record` is empty, it creates a type, and propagates the 
type to `Record->TypeForDecl`;
  - from the above line, we delay the type creation when `IsInjectedClassName` 
is true;
  - so we need to create a type for the `Record` by invoking `getTypeDeclType`;

might be worth a comment.




Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1910
 
+  if (IsInjectedClassName)
+assert(Record->isInjectedClassName() && "Broken injected-class-name");

it is unclear to me what's the intention of the assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1841-1842
+   /*DelayTypeCreation=*/IsInjectedClassName);
+  if (IsInjectedClassName)
+SemaRef.Context.getTypeDeclType(Record, cast(Owner));
 

Why is this call needed? (It seems strange to me that we call it and ignore the 
return value.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: carlosgalvezp.
sammccall added a comment.

After this patch we can revert D110614  
except for the testcases (cc @carlosgalvezp) as the old matchers should just 
work.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:234
 // Derived from template, base has *not* virtual dtor
-// CHECK-MESSAGES: :[[@LINE+8]]:8: warning: destructor of 
'DerivedFromTemplateNonVirtualBaseStruct' is public and non-virtual 
[cppcoreguidelines-virtual-class-destructor]
-// CHECK-MESSAGES: :[[@LINE+7]]:8: note: make it public and virtual

These warnings that disappeared are spurious duplicates. They correspond to 
matching the injected-class-name of the template instantiation (in addition to 
the main declaration of it).
After this change we only match the main declaration, just like non-template 
classes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112765

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

[PATCH] D112765: [AST] injected-class-name is not a redecl, even in template specializations

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: hokein, aaron.ballman, whisperity.
Herald added subscribers: rnkovacs, kbarton, nemanjai.
sammccall requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

Back in the mists of time, the CXXRecordDecl for the injected-class-name was
a redecl of the outer class itself.
This got changed in 470c454a6176ef31474553e408c90f5ee630df89, but only for plain
classes: class template instantation was still detecting the injected-class-name
in the template body and marking its instantiation as a redecl.

This causes some subtle inconsistent behavior between the two, e.g.
hasDefinition() returns true for Foo::Foo but false for Bar::Bar.
This is the root cause of PR51912.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112765

Files:
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-decl.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
  clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp

Index: clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
===
--- clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
+++ clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
@@ -69,7 +69,7 @@
 // CHECK-NEXT: | | | `-Destructor simple irrelevant trivial
 // CHECK-NEXT: | | |-TemplateArgument type 'int'
 // CHECK-NEXT: | | | `-BuiltinType [[ADDR_9:0x[a-z0-9]*]] 'int'
-// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_10:0x[a-z0-9]*]] prev [[ADDR_8]]  col:30 implicit struct S
+// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_10:0x[a-z0-9]*]]  col:30 implicit struct S
 // CHECK-NEXT: | | |-CXXConstructorDecl [[ADDR_11:0x[a-z0-9]*]]  col:3 used S 'void (int, int *)'
 // CHECK-NEXT: | | | |-ParmVarDecl [[ADDR_12:0x[a-z0-9]*]]  col:8 'int'
 // CHECK-NEXT: | | | |-ParmVarDecl [[ADDR_13:0x[a-z0-9]*]]  col:13 'int *'
Index: clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
===
--- clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
+++ clang/test/AST/ast-dump-openmp-begin-declare-variant_reference.cpp
@@ -120,7 +120,7 @@
 // CHECK-NEXT: | | |-TemplateArgument type 'float &'
 // CHECK-NEXT: | | | `-LValueReferenceType [[ADDR_7:0x[a-z0-9]*]] 'float &'
 // CHECK-NEXT: | | |   `-BuiltinType [[ADDR_8:0x[a-z0-9]*]] 'float'
-// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_9:0x[a-z0-9]*]] prev [[ADDR_6]]  col:29 implicit struct remove_reference
+// CHECK-NEXT: | | |-CXXRecordDecl [[ADDR_9:0x[a-z0-9]*]]  col:29 implicit struct remove_reference
 // CHECK-NEXT: | | `-TypedefDecl [[ADDR_10:0x[a-z0-9]*]]  col:67 referenced type 'float':'float'
 // CHECK-NEXT: | |   `-SubstTemplateTypeParmType [[ADDR_11:0x[a-z0-9]*]] 'float' sugar
 // CHECK-NEXT: | | |-TemplateTypeParmType [[ADDR_12:0x[a-z0-9]*]] '_Tp' dependent depth 0 index 0
@@ -137,7 +137,7 @@
 // CHECK-NEXT: |   |-TemplateArgument type 'short &'
 // CHECK-NEXT: |   | `-LValueReferenceType [[ADDR_15:0x[a-z0-9]*]] 'short &'
 // CHECK-NEXT: |   |   `-BuiltinType [[ADDR_16:0x[a-z0-9]*]] 'short'
-// CHECK-NEXT: |   |-CXXRecordDecl [[ADDR_17:0x[a-z0-9]*]] prev [[ADDR_14]]  col:29 implicit struct remove_reference
+// CHECK-NEXT: |   |-CXXRecordDecl [[ADDR_17:0x[a-z0-9]*]]  col:29 implicit struct remove_reference
 // CHECK-NEXT: |   `-TypedefDecl [[ADDR_18:0x[a-z0-9]*]]  col:67 referenced type 'short':'short'
 // CHECK-NEXT: | `-SubstTemplateTypeParmType [[ADDR_19:0x[a-z0-9]*]] 'short' sugar
 // CHECK-NEXT: |   |-TemplateTypeParmType [[ADDR_12]] '_Tp' dependent depth 0 index 0
Index: clang/test/AST/ast-dump-decl.cpp
===
--- clang/test/AST/ast-dump-decl.cpp
+++ clang/test/AST/ast-dump-decl.cpp
@@ -321,7 +321,7 @@
 // CHECK-NEXT:  | |-TemplateArgument type 'testClassTemplateDecl::A'
 // CHECK-NEXT:  | | `-RecordType 0{{.+}} 'testClassTemplateDecl::A'
 // CHECK-NEXT:  | |   `-CXXRecord 0x{{.+}} 'A'
-// CHECK-NEXT:  | |-CXXRecordDecl 0x{{.+}} prev 0x{{.+}}  col:30 implicit class TestClassTemplate
+// CHECK-NEXT:  | |-CXXRecordDecl 0x{{.+}}  col:30 implicit class TestClassTemplate
 // CHECK-NEXT:  | |-AccessSpecDecl 0x{{.+}}  col:3 public
 // CHECK-NEXT:  | |-CXXConstructorDecl 0x{{.+}}  col:5 used TestClassTemplate 'void ()'
 // CHECK-NEXT:  | |-CXXDestructorDecl 0x{{.+}}  col:5 used ~TestClassTemplate 'void () noexcept'
@@ -358,7 +358,7 @@
 // CHECK-NEXT:  |-TemplateArgument type 'testClassTemplateDecl::C'
 // CHECK-NEXT:  | `-RecordType 0{{.+}} 'testClassTemplateDecl::C'
 // CHECK-NEXT:  |   `-CXXRecord 0x{{.+}} 'C'
-// CHECK-NEXT:  |-CXXRecordDecl 0x{{.+}} prev 0x{{.+}}  col:30 implicit class TestClassTemplate
+// CHECK-NEXT:  |-CXXRecordDecl 0x{{.+}}