This is an archive of the discontinued LLVM Phabricator instance.

[clang] Overload NamedDecl::printName to provide a user-friendly name for unnamed entities
AcceptedPublic

Authored by riccibruno on Jul 27 2020, 7:07 AM.

Details

Summary

See D84656 for the background on NamedDecl::printName.

This patch overloads NamedDecl::printName for VarDecl, FieldDecl ,RecordDecl and EnumDecl to provide a more user-friendly name in diagnostics for unnamed entities. We try to only use the term "anonymous" when we have an anonymous struct or union; otherwise we use the term "unnamed".

There is one little tweak we need to make to Sema::BuildAnonymousStructOrUnion: we mark the record as being anonymous as early as possible. This is to ensure that diagnostics emitted here will use the term "anonymous".

Diff Detail

Event Timeline

riccibruno created this revision.Jul 27 2020, 7:07 AM
riccibruno edited the summary of this revision. (Show Details)Jul 27 2020, 7:14 AM

This seems like a strict improvement as well. That said, there are two opportunities to clean up the code that I suggest evaluating.

clang/lib/AST/Decl.cpp
2032

This pattern is happening a bunch too.... Does it deserve some function? Perhaps a function that either takes Policy, or a member of Policy to get the formatting open-tick or close-tick?

2074

It seems this entire block can be pulled out of these two functions. This is probably worth while?

It seems that in this function, it would be:

if (!SuppressLocation)

printLocation(....);

And in the other, just a call to PrintLocation(....).

riccibruno marked 2 inline comments as done.Jul 27 2020, 7:44 AM
riccibruno added inline comments.
clang/lib/AST/Decl.cpp
2032

Do you mean a member function of PrintingPolicy, something like getOpenTick and getCloseTick?

2074

Good point yes. I will do so.

erichkeane added inline comments.Jul 27 2020, 7:46 AM
clang/lib/AST/Decl.cpp
2032

Yeah, exactly, something like that. "Delimiter" is perhaps better than 'tick', but I'm not attached to either.

riccibruno marked an inline comment as done.Jul 27 2020, 7:49 AM
riccibruno added inline comments.
clang/lib/AST/Decl.cpp
2032

"Delimiter" sounds good to me. Thanks for the suggestion!

I think these changes are going in the right direction (and I agree with the feedback from Erich). Thank you for looking into this!

riccibruno marked 3 inline comments as done.

Address Erich's comments.

riccibruno marked an inline comment as done.Jul 27 2020, 1:36 PM
riccibruno added inline comments.
clang/lib/AST/Decl.cpp
2032

I will fix the other instances of this pattern elsewhere as a follow-up.

Note that I am planning two improvements as a follow-up to this patch:

  • For a parameter (unnamed variable at {{.*}}:<line>:<col> of type <type>) -> (unnamed parameter at {{.*}}:<line>:<col> of type <type>)
  • For a lambda:
    • (unnamed class at {{.*}}:<line>:<col>) -> (lambda at {{.*}}:<line>:<col>)
    • For a captured field in the lambda class: (unnamed field at {{.*}}:<line>:<col> of type <type>) -> something which refers to the captured entity instead of the unnamed field.
aaron.ballman added inline comments.Jul 28 2020, 11:42 AM
clang/include/clang/AST/PrettyPrinter.h
78

These names a bit too generic: open/close delimiter for *what* (there's a lot of paired delimiters to consider when pretty printing). Perhaps getUnnamedIdentOpenDelimiter() or something to make it more clear that this isn't, say, an attribute delimiter?

riccibruno edited the summary of this revision. (Show Details)

Use a less generic name instead of get{Open,Close}Delimiter. I went with get{Open,Close}DelimiterForUnnamedEntity but I am happy to change it.

Add the forgotten context.

aaron.ballman accepted this revision.Jul 29 2020, 4:47 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 29 2020, 4:47 AM
shafik added a subscriber: shafik.Sep 26 2023, 2:15 PM

It looks like some of these changes made it through different PRs but a lot of this is not in. It would be nice to get a updated version of this change.

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2023, 2:15 PM