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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D134813#3834938 <https://reviews.llvm.org/D134813#3834938>, @sammccall wrote:

> In D134813#3834776 <https://reviews.llvm.org/D134813#3834776>, @aaron.ballman wrote:
>
>> Thank you for this! I applied D135191 <https://reviews.llvm.org/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]
>>                    ^
>>   <stdin>:386:63: note: scanning from here
>>   // CHECK: usrs.m:3:56: DeclRefExpr=y:3:40 Extent=[3:56 - 3:57]
>>                                                                 ^
>>   <stdin>: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 `<anonymous>` 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 accordingly, or do you think we should land your changes first and then I rebase on top of them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134813