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@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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
