[PATCH] D116387: [CodeCompletion][clangd] Clean __uglified parameter names in completion & hover

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:65
   Base.SuppressTemplateArgsInCXXConstructors = true;
+  Base.CleanUglifiedParameters = true;
   return Base;
----------------
hokein wrote:
> IIUC, the code looks like we change the hover behavior as well, but I think our plan is to not change hover. 
Oops, I messed up the revert. Fixed now!


================
Comment at: clang/lib/AST/DeclPrinter.cpp:889
+  printDeclType(T, (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
+                    D->getIdentifier())
+                       ? D->getIdentifier()->deuglifiedName()
----------------
hokein wrote:
> nit: `D->getIdentifier()` seems redundant -- `D->getName()` has an `isIdentifier()` assertion, we should expect that we can always get identifier from D here. 
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.


================
Comment at: clang/lib/Basic/IdentifierTable.cpp:312
 
+StringRef IdentifierInfo::deuglifiedName() const {
+  StringRef Name = getName();
----------------
hokein wrote:
> 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()`. 
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116387/new/

https://reviews.llvm.org/D116387