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

Tue Oct 4 12:15:08 PDT 2022

aaron.ballman added a comment.

In D134813#3834496 <https://reviews.llvm.org/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 at 102@F at my_helper@y Extent=[3:36 - 3:41]
-// CHECK: usrs.m c:@Ea at ABA Extent=[5:1 - 8:2]
-// CHECK: usrs.m c:@Ea at ABA@ABA Extent=[6:3 - 6:6]
-// CHECK: usrs.m c:@Ea at ABA@CADABA Extent=[7:3 - 7:9]
-// CHECK: usrs.m c:@Ea at FOO Extent=[10:1 - 13:2]
-// CHECK: usrs.m c:@Ea at FOO@FOO Extent=[11:3 - 11:6]
-// CHECK: usrs.m c:@Ea at FOO@BAR Extent=[12:3 - 12:6]
-// CHECK: usrs.m c:@SA at MyStruct Extent=[15:9 - 18:2]
-// CHECK: usrs.m c:@SA at MyStruct@FI at wa Extent=[16:3 - 16:9]
-// CHECK: usrs.m c:@SA at MyStruct@FI at moo Extent=[17:3 - 17:10]
+// CHECK: usrs.m c:@E at enum (unnamed at {{.*}}) Extent=[5:1 - 8:2]
+// CHECK: usrs.m c:@E at 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