[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2023-07-08 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#4482852 , @Hahnfeld wrote:

> In D134813#4482822 , @aaron.ballman 
> wrote:
>
>> In D134813#4482819 , @Hahnfeld 
>> wrote:
>>
>>> Thanks! Note that the same probably holds true (but I didn't test) for all 
>>> other classes that override `printName(raw_ostream , const 
>>> PrintingPolicy )`, ie `DecompositionDecl`, `MSGuidDecl`, 
>>> `UnnamedGlobalConstantDecl`, and `TemplateParamObjectDecl`.
>>
>> You're correct, but I figured those are somewhat uncommon AST nodes, so we 
>> can probably expose those APIs as-needed rather than doing all of them.
>
> Oh, alright.
>
>> However, if you prefer they all get handled, it's easy enough!
>
> No, not needed. I was only running into it with an `EnumDecl` while going to 
> LLVM 16. But as I said, it's easy enough to work around.

In general, I think we want to encourage callers to use the variant with a 
`PrintingPolicy` so that user-controlled options are properly handled (e.g., 
spelling it `_Bool` instead of `bool` older C modes, that sort of thing), so I 
figure the extra work is a subtle encouragement. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2023-07-08 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

In D134813#4482822 , @aaron.ballman 
wrote:

> In D134813#4482819 , @Hahnfeld 
> wrote:
>
>> Thanks! Note that the same probably holds true (but I didn't test) for all 
>> other classes that override `printName(raw_ostream , const PrintingPolicy 
>> )`, ie `DecompositionDecl`, `MSGuidDecl`, 
>> `UnnamedGlobalConstantDecl`, and `TemplateParamObjectDecl`.
>
> You're correct, but I figured those are somewhat uncommon AST nodes, so we 
> can probably expose those APIs as-needed rather than doing all of them.

Oh, alright.

> However, if you prefer they all get handled, it's easy enough!

No, not needed. I was only running into it with an `EnumDecl` while going to 
LLVM 16. But as I said, it's easy enough to work around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2023-07-08 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#4482819 , @Hahnfeld wrote:

> In D134813#4482808 , @aaron.ballman 
> wrote:
>
>> In D134813#4482674 , @Hahnfeld 
>> wrote:
>>
>>> Out of curiosity, not sure if it's worth fixing because it's easy enough to 
>>> work around: I think after this change, it's not possible anymore to call 
>>> `printName(raw_ostream )` on a (statically typed) `TagDecl` / `EnumDecl` 
>>> because it's hidden by `TagDecl::printName(raw_ostream , const 
>>> PrintingPolicy )` while it works perfectly for a `NamedDecl`. Is 
>>> this intentional?
>>
>> That's not intentional, thank you for pointing it out! This should be 
>> corrected in 0566791ab28b5b842c3befea63d7d47fe9ba1a59 
>> 
>
> Thanks! Note that the same probably holds true (but I didn't test) for all 
> other classes that override `printName(raw_ostream , const PrintingPolicy 
> )`, ie `DecompositionDecl`, `MSGuidDecl`, `UnnamedGlobalConstantDecl`, 
> and `TemplateParamObjectDecl`.

You're correct, but I figured those are somewhat uncommon AST nodes, so we can 
probably expose those APIs as-needed rather than doing all of them. However, if 
you prefer they all get handled, it's easy enough!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2023-07-08 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

In D134813#4482808 , @aaron.ballman 
wrote:

> In D134813#4482674 , @Hahnfeld 
> wrote:
>
>> Out of curiosity, not sure if it's worth fixing because it's easy enough to 
>> work around: I think after this change, it's not possible anymore to call 
>> `printName(raw_ostream )` on a (statically typed) `TagDecl` / `EnumDecl` 
>> because it's hidden by `TagDecl::printName(raw_ostream , const 
>> PrintingPolicy )` while it works perfectly for a `NamedDecl`. Is this 
>> intentional?
>
> That's not intentional, thank you for pointing it out! This should be 
> corrected in 0566791ab28b5b842c3befea63d7d47fe9ba1a59 
> 

Thanks! Note that the same probably holds true (but I didn't test) for all 
other classes that override `printName(raw_ostream , const PrintingPolicy 
)`, ie `DecompositionDecl`, `MSGuidDecl`, `UnnamedGlobalConstantDecl`, 
and `TemplateParamObjectDecl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2023-07-08 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#4482674 , @Hahnfeld wrote:

> Out of curiosity, not sure if it's worth fixing because it's easy enough to 
> work around: I think after this change, it's not possible anymore to call 
> `printName(raw_ostream )` on a (statically typed) `TagDecl` / `EnumDecl` 
> because it's hidden by `TagDecl::printName(raw_ostream , const 
> PrintingPolicy )` while it works perfectly for a `NamedDecl`. Is this 
> intentional?

That's not intentional, thank you for pointing it out! This should be corrected 
in 0566791ab28b5b842c3befea63d7d47fe9ba1a59 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2023-07-08 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

Out of curiosity, not sure if it's worth fixing because it's easy enough to 
work around: I think after this change, it's not possible anymore to call 
`printName(raw_ostream )` on a (statically typed) `TagDecl` / `EnumDecl` 
because it's hidden by `TagDecl::printName(raw_ostream , const 
PrintingPolicy )` while it works perfectly for a `NamedDecl`. Is this 
intentional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-14 Thread Aaron Ballman via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG19e984ef8f49: Properly print unnamed TagDecl objects in 
diagnostics (authored by aaron.ballman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

Files:
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTDiagnostic.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/DeclTemplate.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/ExtractAPI/enum.c
  clang/test/Index/annotate-comments-typedef.m
  clang/test/Index/c-index-api-loadTU-test.m
  clang/test/Index/c-index-getCursor-test.m
  clang/test/Index/print-type.c
  clang/test/Index/print-type.cpp
  clang/test/Index/targeted-annotation.c
  clang/test/Index/targeted-cursor.c
  clang/test/Index/usrs.m
  clang/test/Sema/address-packed.c
  clang/test/Sema/attr-flag-enum.c
  clang/test/SemaCXX/attr-unused.cpp
  clang/test/SemaCXX/lambda-expressions.cpp
  clang/test/SemaCXX/ms-interface.cpp
  clang/test/SemaObjCXX/arc-0x.mm
  clang/test/Templight/templight-empty-entries-fix.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  llvm/utils/lit/lit/TestRunner.py

Index: llvm/utils/lit/lit/TestRunner.py
===
--- llvm/utils/lit/lit/TestRunner.py
+++ llvm/utils/lit/lit/TestRunner.py
@@ -1174,6 +1174,7 @@
 ('%/p', sourcedir.replace('\\', '/')),
 ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
 ('%/T', tmpDir.replace('\\', '/')),
+('%/et',tmpName.replace('\\', '')),
 ])
 
 # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
Index: clang/unittests/AST/ASTTraverserTest.cpp
===
--- clang/unittests/AST/ASTTraverserTest.cpp
+++ clang/unittests/AST/ASTTraverserTest.cpp
@@ -1011,7 +1011,7 @@
 | |-FieldDecl ''
 | |-FieldDecl ''
 | |-FieldDecl ''
-| `-CXXDestructorDecl '~'
+| `-CXXDestructorDecl '~(lambda at input.cc:9:3)'
 |-ImplicitCastExpr
 | `-DeclRefExpr 'a'
 |-DeclRefExpr 'b'
Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- clang/test/Templight/templight-empty-entries-fix.cpp
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -5,13 +5,13 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
@@ -225,37 +225,37 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-07 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman updated this revision to Diff 466145.
aaron.ballman added a comment.

Fixes the failing AST unit test in Clang found by precommit CI.


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

https://reviews.llvm.org/D134813

Files:
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTDiagnostic.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/DeclTemplate.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/ExtractAPI/enum.c
  clang/test/Index/annotate-comments-typedef.m
  clang/test/Index/c-index-api-loadTU-test.m
  clang/test/Index/c-index-getCursor-test.m
  clang/test/Index/print-type.c
  clang/test/Index/print-type.cpp
  clang/test/Index/targeted-annotation.c
  clang/test/Index/targeted-cursor.c
  clang/test/Index/usrs.m
  clang/test/Sema/address-packed.c
  clang/test/Sema/attr-flag-enum.c
  clang/test/SemaCXX/attr-unused.cpp
  clang/test/SemaCXX/lambda-expressions.cpp
  clang/test/SemaCXX/ms-interface.cpp
  clang/test/SemaObjCXX/arc-0x.mm
  clang/test/Templight/templight-empty-entries-fix.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  llvm/utils/lit/lit/TestRunner.py

Index: llvm/utils/lit/lit/TestRunner.py
===
--- llvm/utils/lit/lit/TestRunner.py
+++ llvm/utils/lit/lit/TestRunner.py
@@ -1174,6 +1174,7 @@
 ('%/p', sourcedir.replace('\\', '/')),
 ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
 ('%/T', tmpDir.replace('\\', '/')),
+('%/et',tmpName.replace('\\', '')),
 ])
 
 # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
Index: clang/unittests/AST/ASTTraverserTest.cpp
===
--- clang/unittests/AST/ASTTraverserTest.cpp
+++ clang/unittests/AST/ASTTraverserTest.cpp
@@ -1011,7 +1011,7 @@
 | |-FieldDecl ''
 | |-FieldDecl ''
 | |-FieldDecl ''
-| `-CXXDestructorDecl '~'
+| `-CXXDestructorDecl '~(lambda at input.cc:9:3)'
 |-ImplicitCastExpr
 | `-DeclRefExpr 'a'
 |-DeclRefExpr 'b'
Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- clang/test/Templight/templight-empty-entries-fix.cpp
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -5,13 +5,13 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
@@ -225,37 +225,37 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-07 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman updated this revision to Diff 466052.
aaron.ballman added a comment.
Herald added a subscriber: kadircet.
Herald added a project: clang-tools-extra.

Updated to fix the clangd unit test breakage.


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

https://reviews.llvm.org/D134813

Files:
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTDiagnostic.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/DeclTemplate.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/ExtractAPI/enum.c
  clang/test/Index/annotate-comments-typedef.m
  clang/test/Index/c-index-api-loadTU-test.m
  clang/test/Index/c-index-getCursor-test.m
  clang/test/Index/print-type.c
  clang/test/Index/print-type.cpp
  clang/test/Index/targeted-annotation.c
  clang/test/Index/targeted-cursor.c
  clang/test/Index/usrs.m
  clang/test/Sema/address-packed.c
  clang/test/Sema/attr-flag-enum.c
  clang/test/SemaCXX/attr-unused.cpp
  clang/test/SemaCXX/lambda-expressions.cpp
  clang/test/SemaCXX/ms-interface.cpp
  clang/test/SemaObjCXX/arc-0x.mm
  clang/test/Templight/templight-empty-entries-fix.cpp
  llvm/utils/lit/lit/TestRunner.py

Index: llvm/utils/lit/lit/TestRunner.py
===
--- llvm/utils/lit/lit/TestRunner.py
+++ llvm/utils/lit/lit/TestRunner.py
@@ -1174,6 +1174,7 @@
 ('%/p', sourcedir.replace('\\', '/')),
 ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
 ('%/T', tmpDir.replace('\\', '/')),
+('%/et',tmpName.replace('\\', '')),
 ])
 
 # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- clang/test/Templight/templight-empty-entries-fix.cpp
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -5,13 +5,13 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
@@ -225,37 +225,37 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-06 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

Thank you *so much* to @sammccall @zixuw and @dang for the help with indexing 
and API extraction, I really appreciate it!




Comment at: clang/include/clang/AST/Decl.h:3647
 
+  void printName(raw_ostream , const PrintingPolicy ) const;
+

zixuw wrote:
> nit: missing an `override` here. (`warning: 'printName' overrides a member 
> function but is not marked 'override' [-Winconsistent-missing-override]`)
Good catch, thank you!



Comment at: clang/lib/AST/Decl.cpp:4480
+  // the tag is anonymous and we should print it differently.
+  if (Name.isIdentifier() && !Name.getAsIdentifierInfo()) {
+// If the caller wanted to print a qualified name, they've already printed

erichkeane wrote:
> Do we want to assert that this is either an EnumDecl, or a RecordDecl AND 
> RD.isAnonymousStructOrUnion?
It's a reasonable suggestion, but I'd rather not because printing is used by 
the dump methods and dumping is often used from a debugger where things might 
be in a slightly weird state.


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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-06 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman updated this revision to Diff 465825.
aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

Rebased and updated based on review feedback.


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

https://reviews.llvm.org/D134813

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTDiagnostic.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/DeclTemplate.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/ExtractAPI/enum.c
  clang/test/Index/annotate-comments-typedef.m
  clang/test/Index/c-index-api-loadTU-test.m
  clang/test/Index/c-index-getCursor-test.m
  clang/test/Index/print-type.c
  clang/test/Index/print-type.cpp
  clang/test/Index/targeted-annotation.c
  clang/test/Index/targeted-cursor.c
  clang/test/Index/usrs.m
  clang/test/Sema/address-packed.c
  clang/test/Sema/attr-flag-enum.c
  clang/test/SemaCXX/attr-unused.cpp
  clang/test/SemaCXX/lambda-expressions.cpp
  clang/test/SemaCXX/ms-interface.cpp
  clang/test/SemaObjCXX/arc-0x.mm
  clang/test/Templight/templight-empty-entries-fix.cpp
  llvm/utils/lit/lit/TestRunner.py

Index: llvm/utils/lit/lit/TestRunner.py
===
--- llvm/utils/lit/lit/TestRunner.py
+++ llvm/utils/lit/lit/TestRunner.py
@@ -1174,6 +1174,7 @@
 ('%/p', sourcedir.replace('\\', '/')),
 ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
 ('%/T', tmpDir.replace('\\', '/')),
+('%/et',tmpName.replace('\\', '')),
 ])
 
 # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- clang/test/Templight/templight-empty-entries-fix.cpp
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -5,13 +5,13 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
@@ -225,37 +225,37 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK:

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-06 Thread Daniel Grumberg via Phabricator via cfe-commits 
dang accepted this revision.
dang added a comment.

In D134813#3838069 , @zixuw wrote:

> With the PrintingPolicy fix in https://reviews.llvm.org/D135295 and landed 
> USR fix, the diff within ExtractAPI tests is only the wording with anonymous 
> enums, and we can drop the lit change:
>
>   658c658
>   < "spelling": "(anonymous)"
>   ---
>   > "spelling": "enum (unnamed)"
>   661c661
>   < "title": "(anonymous)"
>   ---
>   > "title": "enum (unnamed)"
>   664c664
>   < "(anonymous)"
>   ---
>   > "enum (unnamed)"
>   706c706
>   < "(anonymous)",
>   ---
>   > "enum (unnamed)",
>   746c746
>   < "spelling": "(anonymous)"
>   ---
>   > "spelling": "enum (unnamed)"
>   749c749
>   < "title": "(anonymous)"
>   ---
>   > "title": "enum (unnamed)"
>   752c752
>   < "(anonymous)"
>   ---
>   > "enum (unnamed)"
>   794c794
>   < "(anonymous)",
>   ---
>   > "enum (unnamed)",
>
> @dang @QuietMisdreavus for this change.

As far as I know this should affect downstream consumers like Swift-DocC. I am 
happy with these changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Zixu Wang via Phabricator via cfe-commits 
zixuw added a comment.

With the PrintingPolicy fix in https://reviews.llvm.org/D135295 and landed USR 
fix, the diff within ExtractAPI tests is only the wording with anonymous enums, 
and we can drop the lit change:

  658c658
  < "spelling": "(anonymous)"
  ---
  > "spelling": "enum (unnamed)"
  661c661
  < "title": "(anonymous)"
  ---
  > "title": "enum (unnamed)"
  664c664
  < "(anonymous)"
  ---
  > "enum (unnamed)"
  706c706
  < "(anonymous)",
  ---
  > "enum (unnamed)",
  746c746
  < "spelling": "(anonymous)"
  ---
  > "spelling": "enum (unnamed)"
  749c749
  < "title": "(anonymous)"
  ---
  > "title": "enum (unnamed)"
  752c752
  < "(anonymous)"
  ---
  > "enum (unnamed)"
  794c794
  < "(anonymous)",
  ---
  > "enum (unnamed)",

@dang @QuietMisdreavus for this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Zixu Wang via Phabricator via cfe-commits 
zixuw added a comment.

I'm pulling this on top of https://reviews.llvm.org/D135295 to try locally now.




Comment at: clang/include/clang/AST/Decl.h:3647
 
+  void printName(raw_ostream , const PrintingPolicy ) const;
+

nit: missing an `override` here. (`warning: 'printName' overrides a member 
function but is not marked 'override' [-Winconsistent-missing-override]`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Zixu Wang via Phabricator via cfe-commits 
zixuw added a comment.

Setting `PrintingPolicy::AnonymousTagLocations` to `false` in 
https://reviews.llvm.org/D135295


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Zixu Wang via Phabricator via cfe-commits 
zixuw added a comment.

Ah sorry I just finished reading the discussions. IIUC 
20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e 
 should 
have already fixed the USR generation part, and all of the USR updates in the 
test cases should be gone now?

For the name part (`title`) I believe it's less likely to cause any problem, 
but would still be good to get rid of the location info just for the sake of 
keeping diff working and not having to change lit  (yeah probably using diff 
was not a good idea in the first place.. )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Zixu Wang via Phabricator via cfe-commits 
zixuw added inline comments.



Comment at: clang/test/ExtractAPI/typedef_anonymous_record.c:74
 "interfaceLanguage": "objective-c",
-"precise": "c:@SA@MyStruct"
+"precise": "c:@S@MyStruct"
   },

Why is the `A` dropped here? Isn't this USR still referring to an anonymous 
struct (`typedef struct { } MyStruct;`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Zixu Wang via Phabricator via cfe-commits 
zixuw added a subscriber: QuietMisdreavus.
zixuw added a comment.

In D134813#3836836 , @aaron.ballman 
wrote:

>> I'd guess we need some kind of change to CommentXML and ExtractAPI, but I 
>> don't know enough to be sure what it should be.
>
> Ping @dang @zixuw and @dexonsmith for questions about how to handle 
> ExtractAPI changes, and @gribozavr for questions about CommentXML

Thanks for the heads up! Looking.

At a glance it seems that ExtractAPI would like to fall back to the original 
formats to not break downstream tooling. But not sure if it's fine because it's 
still consistent. As far as I can tell this only affects anonymous `TagDecl`s. 
@QuietMisdreavus thoughts?

IMO we would still want to keep the ExtractAPI outputs unchanged if it's easy 
enough to do. We're using `getName()/getQualifiedNameAsString()` for names and 
`index::generateUSRForDecl()` for USRs. Is there a 'policy' or some 
alternatives to use? Or is the location info now part of the 'correct' format 
for USRs (seems a bit weird to see that as part of the USR)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added subscribers: zixuw, dexonsmith, gribozavr.
aaron.ballman added a comment.

In D134813#3836826 , @sammccall wrote:

> I just landed that change to USR generation as 
> 20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e 
> , so 
> there should now be no USR changes (just some changes to debug 
> representations that happen to be in some of the USR test files, which can be 
> updated).

Thank you!

> I'd guess we need some kind of change to CommentXML and ExtractAPI, but I 
> don't know enough to be sure what it should be.

Ping @dang @zixuw and @dexonsmith for questions about how to handle ExtractAPI 
changes, and @gribozavr for questions about CommentXML


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

I just landed that change to USR generation as 
20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e 
, so there 
should now be no USR changes (just some changes to debug representations that 
happen to be in some of the USR test files, which can be updated).

I'd guess we need some kind of change to CommentXML and ExtractAPI, but I don't 
know enough to be sure what it should be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-05 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#3834938 , @sammccall wrote:

> In D134813#3834776 , @aaron.ballman 
> wrote:
>
>> Thank you for this! I applied D135191  
>> over the top of my changes here and ran the tests to get all the new 
>> failures with the changes, then I reverted those tests which failed back to 
>> their trunk form. When I re-ran the tests, I get the following failures:
>>
>>    TEST 'Clang :: Index/usrs.m' FAILED 
>> 
>>   ...
>>   $ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe" 
>> "-check-prefix=CHECK-source" "F:\source\llvm-project\clang\test\Index\usrs.m"
>>   # command stderr:
>>   F:\source\llvm-project\clang\test\Index\usrs.m:188:18: error: 
>> CHECK-source: expected string not found in input
>>   // CHECK-source: usrs.m:5:1: EnumDecl=:5:1 (Definition) Extent=[5:1 - 8:2]
>>^
>>   :386:63: note: scanning from here
>>   // CHECK: usrs.m:3:56: DeclRefExpr=y:3:40 Extent=[3:56 - 3:57]
>> ^
>>   :387:89: note: possible intended match here
>>   // CHECK: usrs.m:5:1: EnumDecl=enum (unnamed at 
>> F:\source\llvm-project\clang\test\Index\usrs.m:5:1):5:1 (Definition) 
>> Extent=[5:1 - 8:2]
>
> This isn't a change to USRs, it's a change to libclang's 
> `clang_getCursorSpelling`. I think it's safe to update (unlike the USRs 
> earlier in the file).
> Since this is a general-purpose API it makes sense that it would follow the 
> change in printName() defaults.
>
>>    TEST 'Clang :: ExtractAPI/enum.c' FAILED 
>> 
>>   # command output:
>>   *** 
>> F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/reference.output.json
>>   --- 
>> F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output-normalized.json
>>   ***
>>   *** 655,667 
>> "navigator": [
>>   {
>> "kind": "identifier",
>>   ! "spelling": "(anonymous)"
>>   !   }
>>   ! ],
>>   ! "title": "(anonymous)"
>>   !   },
>>   !   "pathComponents": [
>>   ! "(anonymous)"
>>   ]
>> },
>> {
>>   --- 655,667 
>> "navigator": [
>>   {
>> "kind": "identifier",
>>   ! "spelling": "enum (unnamed at 
>> F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>>   !   }
>>   ! ],
>>   ! "title": "enum (unnamed at 
>> F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>>   !   },
>>   !   "pathComponents": [
>>   ! "enum (unnamed at 
>> F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>>   ]
>> },
>>   ...
>
> ExtractAPI seems to produce a data description of the AST for programmatic 
> consumption (apple swift interop?), this looks like a breaking change to me.
> It looks like they have at least one explicit check similar to USRs in 
> clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:361, but I know very little about 
> how this tool is used out-of-tree by Apple: I'm not sure how much the exact 
> strings/lack of strings matters, may need owners of that tool involved here.
>
>>    TEST 'Clang :: Index/annotate-comments-typedef.m' 
>> FAILED 
>
> I can't see the actual output of the tool in that log, but I guess this is 
> related to clang/lib/Index/CommentToXML.cpp:908.
> I don't know if reporting exactly `` is important behavior that 
> should be preserved, or `(unnamed)` would be fine. I don't even know what 
> CommentToXML is: again I guess it's consumed by something related to XCode 
> out-of-tree.
>
>> and also the clangd unit tests failed:
>>
>>   [--] 1 test from FindExplicitReferencesTest
>>   [ RUN  ] FindExplicitReferencesTest.All
>>   ...
>>   With diff:
>>   @@ -1,3 +1,3 @@
>>   -0: targets = {}
>>   +0: targets = {(unnamed)}
>>1: targets = {x}, decl
>>2: targets = {fptr}, decl
>>   
>>   
>>void foo() {
>> $0^class {} $1^x;
>> int (*$2^fptr)(int $3^a, int) = nullptr;
>>}
>
> Yes, this is expected: the test identifies which target is referenced by 
> name, and we changed the name from "" to "(unnamed)".
> This patch can change the test data on 
> clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1619 from `targets = 
> {}` to `targets={(unnamed)}`.

Thank you for the extra analysis! In terms of next steps, do you think I should 
roll your changes into this review and adjust the tests

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

In D134813#3834776 , @aaron.ballman 
wrote:

> Thank you for this! I applied D135191  over 
> the top of my changes here and ran the tests to get all the new failures with 
> the changes, then I reverted those tests which failed back to their trunk 
> form. When I re-ran the tests, I get the following failures:
>
>    TEST 'Clang :: Index/usrs.m' FAILED 
> 
>   ...
>   $ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe" 
> "-check-prefix=CHECK-source" "F:\source\llvm-project\clang\test\Index\usrs.m"
>   # command stderr:
>   F:\source\llvm-project\clang\test\Index\usrs.m:188:18: error: CHECK-source: 
> expected string not found in input
>   // CHECK-source: usrs.m:5:1: EnumDecl=:5:1 (Definition) Extent=[5:1 - 8:2]
>^
>   :386:63: note: scanning from here
>   // CHECK: usrs.m:3:56: DeclRefExpr=y:3:40 Extent=[3:56 - 3:57]
> ^
>   :387:89: note: possible intended match here
>   // CHECK: usrs.m:5:1: EnumDecl=enum (unnamed at 
> F:\source\llvm-project\clang\test\Index\usrs.m:5:1):5:1 (Definition) 
> Extent=[5:1 - 8:2]

This isn't a change to USRs, it's a change to libclang's 
`clang_getCursorSpelling`. I think it's safe to update (unlike the USRs earlier 
in the file).
Since this is a general-purpose API it makes sense that it would follow the 
change in printName() defaults.

>    TEST 'Clang :: ExtractAPI/enum.c' FAILED 
> 
>   # command output:
>   *** 
> F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/reference.output.json
>   --- 
> F:\source\llvm-project\llvm\out\build\x64-Debug\tools\clang\test\ExtractAPI\Output\enum.c.tmp/output-normalized.json
>   ***
>   *** 655,667 
> "navigator": [
>   {
> "kind": "identifier",
>   ! "spelling": "(anonymous)"
>   !   }
>   ! ],
>   ! "title": "(anonymous)"
>   !   },
>   !   "pathComponents": [
>   ! "(anonymous)"
>   ]
> },
> {
>   --- 655,667 
> "navigator": [
>   {
> "kind": "identifier",
>   ! "spelling": "enum (unnamed at 
> F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>   !   }
>   ! ],
>   ! "title": "enum (unnamed at 
> F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>   !   },
>   !   "pathComponents": [
>   ! "enum (unnamed at 
> F:\\source\\llvm-project\\llvm\\out\\build\\x64-Debug\\tools\\clang\\test\\ExtractAPI\\Output\\enum.c.tmp/input.h:17:1)"
>   ]
> },
>   ...

ExtractAPI seems to produce a data description of the AST for programmatic 
consumption (apple swift interop?), this looks like a breaking change to me.
It looks like they have at least one explicit check similar to USRs in 
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:361, but I know very little about 
how this tool is used out-of-tree by Apple: I'm not sure how much the exact 
strings/lack of strings matters, may need owners of that tool involved here.

>    TEST 'Clang :: Index/annotate-comments-typedef.m' 
> FAILED 

I can't see the actual output of the tool in that log, but I guess this is 
related to clang/lib/Index/CommentToXML.cpp:908.
I don't know if reporting exactly `` is important behavior that 
should be preserved, or `(unnamed)` would be fine. I don't even know what 
CommentToXML is: again I guess it's consumed by something related to XCode 
out-of-tree.

> and also the clangd unit tests failed:
>
>   [--] 1 test from FindExplicitReferencesTest
>   [ RUN  ] FindExplicitReferencesTest.All
>   ...
>   With diff:
>   @@ -1,3 +1,3 @@
>   -0: targets = {}
>   +0: targets = {(unnamed)}
>1: targets = {x}, decl
>2: targets = {fptr}, decl
>   
>   
>void foo() {
> $0^class {} $1^x;
> int (*$2^fptr)(int $3^a, int) = nullptr;
>}

  Yes, this is expected: the test identifies which target is referenced by 
name, and we changed the name from "" to "(unnamed)".
  This patch can change the test data on 
clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1619 from `targets = {}` 
to `targets={(unnamed)}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#3834710 , @sammccall wrote:

> In D134813#3834613 , @sammccall 
> wrote:
>
>> Changing USR generation to not rely on this detail seems easier, I can take 
>> a stab at this.
>
> Seems trivial: https://reviews.llvm.org/D135191
> If there are still diffs in USR tests after rebasing on that, I'm happy to 
> take a look at them.

Thank you for this! I applied D135191  over 
the top of my changes here and ran the tests to get all the new failures with 
the changes, then I reverted those tests which failed back to their trunk form. 
When I re-ran the tests, I get the following failures:

  F:\source\llvm-project>python llvm\out\build\x64-Debug\bin\llvm-lit.py -sv 
-j61 clang\test\
  llvm-lit.py: F:\source\llvm-project\llvm\utils\lit\lit\llvm\config.py:46: 
note: using lit tools: C:\GnuWin32\bin
  llvm-lit.py: F:\source\llvm-project\llvm\utils\lit\lit\llvm\config.py:456: 
note: using clang: f:\source\llvm-project\llvm\out\build\x64-debug\bin\clang.exe
  llvm-lit.py: F:\source\llvm-project\llvm\utils\lit\lit\discovery.py:247: 
warning: test suite 'Clang-Unit' contained no tests
  -- Testing: 15539 tests, 61 workers --
  Testing:
  FAIL: Clang :: Index/usrs.m (1 of 15539)
   TEST 'Clang :: Index/usrs.m' FAILED 
  Script:
  --
  : 'RUN: at line 103';   
f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe 
-test-load-source-usrs all -target x86_64-apple-macosx10.7 
F:\source\llvm-project\clang\test\Index\usrs.m -isystem 
F:\source\llvm-project\clang\test\Index/Inputs | 
f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe 
F:\source\llvm-project\clang\test\Index\usrs.m
  : 'RUN: at line 173';   
f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe 
-test-load-source all F:\source\llvm-project\clang\test\Index\usrs.m -isystem 
F:\source\llvm-project\clang\test\Index/Inputs | 
f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe 
-check-prefix=CHECK-source F:\source\llvm-project\clang\test\Index\usrs.m
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "RUN: at line 103"
  $ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe" 
"-test-load-source-usrs" "all" "-target" "x86_64-apple-macosx10.7" 
"F:\source\llvm-project\clang\test\Index\usrs.m" "-isystem" 
"F:\source\llvm-project\clang\test\Index/Inputs"
  # command stderr:
  F:\source\llvm-project\clang\test\Index\usrs.m:25:12: warning: class 'Foo' 
defined without specifying a base class [-Wobjc-root-class]
  Number FIX-ITs = 0
  F:\source\llvm-project\clang\test\Index\usrs.m:25:15: note: add a super class 
to fix this problem
  Number FIX-ITs = 0
  F:\source\llvm-project\clang\test\Index\usrs.m:51:12: warning: class 
'CWithExt' defined without specifying a base class [-Wobjc-root-class]
  Number FIX-ITs = 0
  F:\source\llvm-project\clang\test\Index\usrs.m:51:20: note: add a super class 
to fix this problem
  Number FIX-ITs = 0
  F:\source\llvm-project\clang\test\Index\usrs.m:101:9: warning: 'MACRO3' macro 
redefined [-Wmacro-redefined]
  Number FIX-ITs = 0
  F:\source\llvm-project\clang\test\Index\usrs.m:100:9: note: previous 
definition is here
  Number FIX-ITs = 0
  
  $ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\filecheck.exe" 
"F:\source\llvm-project\clang\test\Index\usrs.m"
  $ ":" "RUN: at line 173"
  $ "f:\source\llvm-project\llvm\out\build\x64-debug\bin\c-index-test.exe" 
"-test-load-source" "all" "F:\source\llvm-project\clang\test\Index\usrs.m" 
"-isystem" "F:\source\llvm-project\clang\test\Index/Inputs"
  # command stderr:
  F:\source\llvm-project\clang\test\Index\usrs.m:44:13: error: synthesized 
property 'd1' must either be named the same as a compatible instance variable 
or must explicitly name an instance variable
  F:\source\llvm-project\clang\test\Index\usrs.m:25:12: warning: class 'Foo' 
defined without specifying a base class [-Wobjc-root-class]
  F:\source\llvm-project\clang\test\Index\usrs.m:25:15: note: add a super class 
to fix this problem
  F:\source\llvm-project\clang\test\Index\usrs.m:51:12: warning: class 
'CWithExt' defined without specifying a base class [-Wobjc-root-class]
  F:\source\llvm-project\clang\test\Index\usrs.m:51:20: note: add a super class 
to fix this problem
  F:\source\llvm-project\clang\test\Index\usrs.m:86:6: error: instance 
variables may not be placed in class extension
  F:\source\llvm-project\clang\test\Index\usrs.m:101:9: warning: 'MACRO3' macro 
redefined [-Wmacro-redefined]
  F:\source\llvm-project\clang\test\Index\usrs.m:100:9: note: previous 
definition is here
  F:\source\llvm-project\clang\test\Index\usrs.m:44:13: error: synthesized 
property 'd1' must either be named the same as a compatible instance variable 
or must explicitly name an instance variable
  Number FIX-ITs = 0

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

In D134813#3834613 , @sammccall wrote:

> Changing USR generation to not rely on this detail seems easier, I can take a 
> stab at this.

Seems trivial: https://reviews.llvm.org/D135191
If there are still diffs in USR tests after rebasing on that, I'm happy to take 
a look at them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#3834613 , @sammccall wrote:

> In D134813#3834540 , @aaron.ballman 
> wrote:
>
>> In D134813#3834496 , @sammccall 
>> wrote:
>>
>>> Sorry, Monday was a holiday here...
>>
>> No worries, I hope you had a good holiday!
>
> Thanks :-)
>
>>> I don't think unconditionally including the filename in 
>>> printQualifiedName() is great for tools, it can be unreasonably long and is 
>>> generally just noise when shown in the context of that file. I'm surprised 
>>> that USR generation + that clangd test are the only things that broke! Poor 
>>> test coverage in the tools, I think :-(
>>> If the intent is to change this for diagnostics only, can it be behind a 
>>> PrintingPolicy flag?
>>
>> Diagnostics are largely the intent for this and I could probably put a 
>> printing policy flag, but I'm not yet convinced that printing nothing is 
>> actually better for tools in general.
>
> Agreed - I think in my perfect world we'd switch between `(unnammed struct)` 
> and `(unnamed struct at ...)`.
> ... wait, we already have `PrintingPolicy::AnonymousTagLocations`, looks like 
> that does what I want. I've set this for clangd in 
> e212a4f838f17e2d37b1d572d8c1b49c50d7fe17 
> , so 
> this patch should be fine with just updating the string in the test to 
> `(unnamed class)`.

Hehe, I was getting a start on your idea when I discovered that as well. :-D

>> I think it really boils down to whether the name is user-facing or not. 
>> e.g., for USRs, it seems like we don't want to print this sort of thing, 
>> which is fine because users don't stare at those. But tools like clang-query 
>> output various names of things based on user queries, and it seems like it's 
>> less useful for that output to print nothing for the name.
>
> Yeah, I can't see a case where silently printing *nothing* is clearly what we 
> want.
> Even USRs mostly don't do that: they try to print the name, detect nothing 
> got printed, and print something else instead!
>
>> That said, it still sounds like we should have a printing policy for it, but 
>> what should the default be? (I lean towards "print more information instead 
>> of less".)
>
> Given that we have a way to suppress the filename, I don't think we should 
> add another printing policy (until we see a reason to).
> Changing USR generation to not rely on this detail seems easier, I can take a 
> stab at this.

Thank you, I think that's a good idea. I'll rebase on top of those changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

In D134813#3834540 , @aaron.ballman 
wrote:

> In D134813#3834496 , @sammccall 
> wrote:
>
>> Sorry, Monday was a holiday here...
>
> No worries, I hope you had a good holiday!

Thanks :-)

>> I don't think unconditionally including the filename in printQualifiedName() 
>> is great for tools, it can be unreasonably long and is generally just noise 
>> when shown in the context of that file. I'm surprised that USR generation + 
>> that clangd test are the only things that broke! Poor test coverage in the 
>> tools, I think :-(
>> If the intent is to change this for diagnostics only, can it be behind a 
>> PrintingPolicy flag?
>
> Diagnostics are largely the intent for this and I could probably put a 
> printing policy flag, but I'm not yet convinced that printing nothing is 
> actually better for tools in general.

Agreed - I think in my perfect world we'd switch between `(unnammed struct)` 
and `(unnamed struct at ...)`.
... wait, we already have `PrintingPolicy::AnonymousTagLocations`, looks like 
that does what I want. I've set this for clangd in 
e212a4f838f17e2d37b1d572d8c1b49c50d7fe17 
, so this 
patch should be fine with just updating the string in the test to `(unnamed 
class)`.

> I think it really boils down to whether the name is user-facing or not. e.g., 
> for USRs, it seems like we don't want to print this sort of thing, which is 
> fine because users don't stare at those. But tools like clang-query output 
> various names of things based on user queries, and it seems like it's less 
> useful for that output to print nothing for the name.

Yeah, I can't see a case where silently printing *nothing* is clearly what we 
want.
Even USRs mostly don't do that: they try to print the name, detect nothing got 
printed, and print something else instead!

> That said, it still sounds like we should have a printing policy for it, but 
> what should the default be? (I lean towards "print more information instead 
> of less".)

Given that we have a way to suppress the filename, I don't think we should add 
another printing policy (until we see a reason to).
Changing USR generation to not rely on this detail seems easier, I can take a 
stab at this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D134813#3834496 , @sammccall wrote:

> Sorry, Monday was a holiday here...

No worries, I hope you had a good holiday!

> I don't think unconditionally including the filename in printQualifiedName() 
> is great for tools, it can be unreasonably long and is generally just noise 
> when shown in the context of that file. I'm surprised that USR generation + 
> that clangd test are the only things that broke! Poor test coverage in the 
> tools, I think :-(
> If the intent is to change this for diagnostics only, can it be behind a 
> PrintingPolicy flag?

Diagnostics are largely the intent for this and I could probably put a printing 
policy flag, but I'm not yet convinced that printing nothing is actually better 
for tools in general. I think it really boils down to whether the name is 
user-facing or not. e.g., for USRs, it seems like we don't want to print this 
sort of thing, which is fine because users don't stare at those. But tools like 
clang-query output various names of things based on user queries, and it seems 
like it's less useful for that output to print nothing for the name.

That said, it still sounds like we should have a printing policy for it, but 
what should the default be? (I lean towards "print more information instead of 
less".)

> The reason USR generation broke is it was deliberately testing/handling, see 
> `bool USRGenerator::EmitDeclName()` in USRGeneration.cpp. (I'm not very 
> familiar with the implementation, a bit more so with how its results are 
> used). This isn't a good reason to keep the output as `""` forever - we 
> should probably change it to detect empty names explicitly instead.

Ah, thank you for the details!




Comment at: clang/test/Index/usrs.m:113
 // CHECK: usrs.m c:usrs.m@102@F@my_helper@y Extent=[3:36 - 3:41]
-// CHECK: usrs.m c:@Ea@ABA Extent=[5:1 - 8:2]
-// CHECK: usrs.m c:@Ea@ABA@ABA Extent=[6:3 - 6:6]
-// CHECK: usrs.m c:@Ea@ABA@CADABA Extent=[7:3 - 7:9]
-// CHECK: usrs.m c:@Ea@FOO Extent=[10:1 - 13:2]
-// CHECK: usrs.m c:@Ea@FOO@FOO Extent=[11:3 - 11:6]
-// CHECK: usrs.m c:@Ea@FOO@BAR Extent=[12:3 - 12:6]
-// CHECK: usrs.m c:@SA@MyStruct Extent=[15:9 - 18:2]
-// CHECK: usrs.m c:@SA@MyStruct@FI@wa Extent=[16:3 - 16:9]
-// CHECK: usrs.m c:@SA@MyStruct@FI@moo Extent=[17:3 - 17:10]
+// CHECK: usrs.m c:@E@enum (unnamed at {{.*}}) Extent=[5:1 - 8:2]
+// CHECK: usrs.m c:@E@enum (unnamed at {{.*}})@ABA Extent=[6:3 - 6:6]

sammccall wrote:
> This changes the semantics of the USRs: previously if an anonymous enum moved 
> (e.g. because a file was edited) it retains its identity as long as the first 
> enumerator keeps its name.
> 
> The equivalence classes of decls under USR mapping is important for some 
> tools: I don't know all the ways Apple/XCode stuff uses it, but refactoring 
> tools, clangd indexing etc will be affected by this.
> 
> (Occasional changes to the concrete USR values are manageable, basically we 
> need to invalidate indexes that are otherwise usable across versions)
Thank you for the extra set of eyes on this test, that's really good 
information!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

Sorry, Monday was a holiday here...

I don't think unconditionally including the filename in printQualifiedName() is 
great for tools, it can be unreasonably long and is generally just noise when 
shown in the context of that file. I'm surprised that USR generation + that 
clangd test are the only things that broke! Poor test coverage in the tools, I 
think :-(
If the intent is to change this for diagnostics only, can it be behind a 
PrintingPolicy flag?

The reason USR generation broke is it was deliberately testing/handling, see 
`bool USRGenerator::EmitDeclName()` in USRGeneration.cpp. (I'm not very 
familiar with the implementation, a bit more so with how its results are used). 
This isn't a good reason to keep the output as `""` forever - we should 
probably change it to detect empty names explicitly instead.




Comment at: clang/test/Index/usrs.m:113
 // CHECK: usrs.m c:usrs.m@102@F@my_helper@y Extent=[3:36 - 3:41]
-// CHECK: usrs.m c:@Ea@ABA Extent=[5:1 - 8:2]
-// CHECK: usrs.m c:@Ea@ABA@ABA Extent=[6:3 - 6:6]
-// CHECK: usrs.m c:@Ea@ABA@CADABA Extent=[7:3 - 7:9]
-// CHECK: usrs.m c:@Ea@FOO Extent=[10:1 - 13:2]
-// CHECK: usrs.m c:@Ea@FOO@FOO Extent=[11:3 - 11:6]
-// CHECK: usrs.m c:@Ea@FOO@BAR Extent=[12:3 - 12:6]
-// CHECK: usrs.m c:@SA@MyStruct Extent=[15:9 - 18:2]
-// CHECK: usrs.m c:@SA@MyStruct@FI@wa Extent=[16:3 - 16:9]
-// CHECK: usrs.m c:@SA@MyStruct@FI@moo Extent=[17:3 - 17:10]
+// CHECK: usrs.m c:@E@enum (unnamed at {{.*}}) Extent=[5:1 - 8:2]
+// CHECK: usrs.m c:@E@enum (unnamed at {{.*}})@ABA Extent=[6:3 - 6:6]

This changes the semantics of the USRs: previously if an anonymous enum moved 
(e.g. because a file was edited) it retains its identity as long as the first 
enumerator keeps its name.

The equivalence classes of decls under USR mapping is important for some tools: 
I don't know all the ways Apple/XCode stuff uses it, but refactoring tools, 
clangd indexing etc will be affected by this.

(Occasional changes to the concrete USR values are manageable, basically we 
need to invalidate indexes that are otherwise usable across versions)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-10-04 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a subscriber: mizvekov.
aaron.ballman added a comment.

In D134813#3826991 , @aaron.ballman 
wrote:

> The clangd test failure found by precommit CI is stumping me as to how to 
> resolve it. @sammccall -- can you help me out? The issue is that the test is 
> expecting no name but now we're printing more information, but that 
> information includes a file path which may differ between machines.

Ping @sammccall. FWIW, I found that the failing part of this test was change 
somewhat recently in 
https://github.com/llvm/llvm-project/commit/15f3cd6bfc670ba6106184a903eb04be059e5977
 to add the markings to print the name of this class, so also pinging @mizvekov 
to see if this test coverage is necessary (another way to solve the issue is to 
stop trying to read the name of the unnamed class in the unit test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-09-30 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a subscriber: sammccall.
aaron.ballman added a comment.

The clangd test failure found by precommit CI is stumping me as to how to 
resolve it. @sammccall -- can you help me out? The issue is that the test is 
expecting no name but now we're printing more information, but that information 
includes a file path which may differ between machines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-09-28 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: clang/lib/AST/Decl.cpp:4480
+  // the tag is anonymous and we should print it differently.
+  if (Name.isIdentifier() && !Name.getAsIdentifierInfo()) {
+// If the caller wanted to print a qualified name, they've already printed

Do we want to assert that this is either an EnumDecl, or a RecordDecl AND 
RD.isAnonymousStructOrUnion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-09-28 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/test/ExtractAPI/enum.c:3
 // RUN: split-file %s %t
-// RUN: sed -e "s@INPUT_DIR@%{/t:regex_replacement}@g" \
+// RUN: sed -e "s@INPUT_FILE@%/et/input.h@g" -e 
"s@INPUT_DIR@%{/t:regex_replacement}@g" \
 // RUN: %t/reference.output.json.in >> %t/reference.output.json

This test's use of `diff` makes the diagnostic changes rather challenging. We 
can't diff the file path that's included when printing an unnamed object, 
because that path may be different from machine to machine. `diff` doesn't have 
any wildcard matching functionality to help here either. So we use `sed` to 
mutate the test file.. but the output uses the path of `%t` with escapes and we 
don't have a sed-compatible way to do that on Windows.

e.g., the output we want to match will contain 
`F:\\users\\aballman\\desktop\\test.c` on Windows but `%t` gives 
`F:\users\aballman\desktop\test.c` which sed turns into `F:sers ballman esktop 
est.c` or some other garbled form.

I'm hoping someone has a better way to approach this. I added `%/et` (for 
escaped %t) to get the behavior I needed, but I'd prefer not to modify lit for 
this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813

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

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

2022-09-28 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman created this revision.
aaron.ballman added reviewers: erichkeane, jyknight.
Herald added subscribers: arphaman, delcypher.
Herald added a reviewer: dang.
Herald added a reviewer: ributzka.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: llvm-commits.

The diagnostics engine is very smart about being passed a `NamedDecl` to print 
as part of a diagnostic; it gets the "right" form of the name, quotes it 
properly, etc. However, the result of using an unnamed tag declaration was to 
print `''` instead of anything useful.

This patch causes us to print the same information we'd have gotten if we had 
printed the type of the declaration rather than the name of it, as that's the 
most relevant information we can display.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134813

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclTemplate.h
  clang/lib/AST/ASTDiagnostic.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/DeclTemplate.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/Analysis/func-mapping-test.cpp
  clang/test/ExtractAPI/enum.c
  clang/test/ExtractAPI/typedef_anonymous_record.c
  clang/test/ExtractAPI/underscored.c
  clang/test/Index/Core/index-source.m
  clang/test/Index/annotate-comments-typedef.m
  clang/test/Index/c-index-api-loadTU-test.m
  clang/test/Index/c-index-getCursor-test.m
  clang/test/Index/cxx11-lambdas.cpp
  clang/test/Index/cxx14-lambdas.cpp
  clang/test/Index/print-type.c
  clang/test/Index/print-type.cpp
  clang/test/Index/targeted-annotation.c
  clang/test/Index/targeted-cursor.c
  clang/test/Index/usrs.cpp
  clang/test/Index/usrs.m
  clang/test/Sema/address-packed.c
  clang/test/Sema/attr-flag-enum.c
  clang/test/SemaCXX/attr-unused.cpp
  clang/test/SemaCXX/lambda-expressions.cpp
  clang/test/SemaCXX/ms-interface.cpp
  clang/test/SemaObjCXX/arc-0x.mm
  clang/test/Templight/templight-empty-entries-fix.cpp
  llvm/utils/lit/lit/TestRunner.py

Index: llvm/utils/lit/lit/TestRunner.py
===
--- llvm/utils/lit/lit/TestRunner.py
+++ llvm/utils/lit/lit/TestRunner.py
@@ -1174,6 +1174,7 @@
 ('%/p', sourcedir.replace('\\', '/')),
 ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
 ('%/T', tmpDir.replace('\\', '/')),
+('%/et',tmpName.replace('\\', '')),
 ])
 
 # "%{/[STpst]:regex_replacement}" should be normalized like "%/[STpst]" but we're
Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- clang/test/Templight/templight-empty-entries-fix.cpp
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -5,13 +5,13 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^name:[ ]+'\(lambda at .*templight-empty-entries-fix.cpp:4:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
@@ -225,37 +225,37 @@
 }
 
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+End$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed struct$}}
+// CHECK: {{^name:[ ]+'\(unnamed struct at .*templight-empty-entries-fix.cpp:223:3\)'$}}
 // CHECK: {{^kind:[ ]+Memoization$}}
 // CHECK: {{^event:[ ]+Begin$}}
 // CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:223:3'$}}
 // CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:224:5'$}}
 // CHECK-LABEL: {{^---$}}
-// CHECK: {{^name:[ ]+unnamed