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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
