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

Tue Oct 4 12:55:56 PDT 2022

aaron.ballman added a comment.

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

> In D134813#3834540 <https://reviews.llvm.org/D134813#3834540>, @aaron.ballman wrote:
>
>> 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!
>
> 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 <https://reviews.llvm.org/rGe212a4f838f17e2d37b1d572d8c1b49c50d7fe17>, 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