Page MenuHomePhabricator

[CodeCompletion][clangd] Clean __uglified parameter names in completion & hover
ClosedPublic

Authored by sammccall on Dec 29 2021, 5:51 PM.

Details

Summary

Underscore-uglified identifiers are used in standard library implementations to
guard against collisions with macros, and they hurt readability considerably.
(Consider push_back(Tp_ &&__value) vs push_back(Tp value).
When we're describing an interface, the exact names of parameters are not
critical so we can drop these prefixes.

This patch adds a new PrintingPolicy flag that can applies this stripping
when recursively printing pieces of AST.
We set it in code completion/signature help, and in clangd's hover display.
All three features also do a bit of manual poking at names, so fix up those too.

Fixes https://github.com/clangd/clangd/issues/736

Diff Detail

Event Timeline

sammccall created this revision.Dec 29 2021, 5:51 PM
sammccall requested review of this revision.Dec 29 2021, 5:51 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 29 2021, 5:51 PM
nridge added a subscriber: nridge.Jan 1 2022, 5:00 PM

Agree that __ reserved names are ugly, but I'm not sure this is a great improvement.

I played around the patch locally, and found the new behavior being confused in some cases (mostly inconsistencies between deuglified places vs uglified places), and seems hard for readers to predict it:

  • inconsistency with doc-comment which still uses the __ name (this seems hard to fix)

  • the print type in hover still uses __name (maybe this is expected, or we could introduce a reserved field in hover, like template-type-param Tp (reserved))

  • the deuglified behavior is triggered on (template/function) parameters, which means we have uglified name for void foo(_ReservedClass& abc) vs deuglified name for template<typename _ReservedClass> void foo(_ReservedClass& abc) (expanding the scope could fix that, but it doesn't seem something we want to do)

Agree that __ reserved names are ugly, but I'm not sure this is a great improvement.

I played around the patch locally, and found the new behavior being confused in some cases (mostly inconsistencies between deuglified places vs uglified places), and seems hard for readers to predict it:

Agree, but I don't think it needs to be predictable, it's enough that the output can be understood.
(e.g. I never particularly noticed *which* identifiers in stdlib were ugly, just that the thing overall was hard to read).
i.e. if we remove the underscores half the time, that seems like a win.

  • inconsistency with doc-comment which still uses the __ name (this seems hard to fix)

Yes :-( However I don't think a human reader is likely to be confused by this.

  • the print type in hover still uses __name (maybe this is expected, or we could introduce a reserved field in hover, like template-type-param Tp (reserved))

This is an oversight, I'll fix this.

  • the deuglified behavior is triggered on (template/function) parameters, which means we have uglified name for void foo(_ReservedClass& abc) vs deuglified name for template<typename _ReservedClass> void foo(_ReservedClass& abc) (expanding the scope could fix that, but it doesn't seem something we want to do)

This is deliberate: _ReservedClass is part of the API of foo, so renaming it is a semantic change.
I don't this change increases the amount of inconsistency: today we have`vector` vs push_back vs _ReservedClass vs _Tp.

sammccall updated this revision to Diff 398779.Jan 10 2022, 4:41 PM

Fix printName

sammccall updated this revision to Diff 398991.Jan 11 2022, 9:37 AM

Revert hover changes, only use for code complete

hokein added inline comments.Jan 24 2022, 4:22 AM
clang-tools-extra/clangd/Hover.cpp
65 ↗(On Diff #398991)

IIUC, the code looks like we change the hover behavior as well, but I think our plan is to not change hover.

clang/lib/AST/DeclPrinter.cpp
889

nit: D->getIdentifier() seems redundant -- D->getName() has an isIdentifier() assertion, we should expect that we can always get identifier from D here.

1138

isa<TemplateTemplateParmDecl>(D) is redundant as line 1128 has checked it.

clang/lib/Basic/IdentifierTable.cpp
312

Not objecting the current approach -- an alternative is to incline this into getName by adding a parameter Deuglify to control the behavior.

Not sure it is much better, but it seem to save some boilerplate code like Policy.CleanUglifiedParameters ? II->deuglifiedName() : II->getName().

sammccall marked 2 inline comments as done.Jan 25 2022, 11:51 PM
sammccall added inline comments.
clang-tools-extra/clangd/Hover.cpp
65 ↗(On Diff #398991)

Oops, I messed up the revert. Fixed now!

clang/lib/AST/DeclPrinter.cpp
889

Sadly not: if D has no name (e.g. the param in void foo(int)) then isIdentifier() is true but getIdentifier() is nullptr :-(

This is a big trap in the API, but not one I can easily fix here.

clang/lib/Basic/IdentifierTable.cpp
312

Yeah, I was concerned about putting this in the way of a critical/central function.

If we were to do this maybe we should choose a spelling like getDeuglifiedName(bool Deuglify=true).

I'm not sure this is so much better, as it only helps some fraction of the callsites (not the ones that need to fall back to printing the whole DeclarationName instead, and not the one that want to use Policy.CleanUglifyParameters to short-circuit some other check instead)

sammccall marked an inline comment as done.

Fix hover revert.

hokein accepted this revision.Jan 26 2022, 6:28 AM
This revision is now accepted and ready to land.Jan 26 2022, 6:28 AM
This revision was landed with ongoing or failed builds.Jan 26 2022, 6:51 AM
This revision was automatically updated to reflect the committed changes.