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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
