[PATCH] D138425: [clangd] Parameter hints for template specialization

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The code looks pretty good and this makes a lot of logical sense.

However, there's an ugly practical issue: it's overwhelmingly common for template parameters to have cryptic and useless names.
It seems like turning `vector<int>` into `vector</*Tp: */int>` actually makes things worse.

And this gets yet worse: because this is type `parameter` and controlled by that config option, so people can't disable this without disabling the (very practically useful) function param hints.

Have you been using this for a while? Do you find it to be a practical help? Spammy?



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:378
 
+  bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc L) {
+    auto TP = L.getTypePtr()
----------------
we need a bailout `if (!Cfg.InlayHints.Parameters)`


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:434
+        break;
+      if (auto Name = TP[I]->getName(); shouldHintName(TA[I], Name))
+        addInlayHint(TA[I].getSourceRange(), HintSide::Left,
----------------
we're missing some mangling of the name to remove the leading `_`
(stripLeadingUnderscore?)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:553
+    llvm::raw_string_ostream Out(ArgContent);
+    TA.getArgument().print(AST.getPrintingPolicy(), Out, false);
+    if (ArgContent == ParamName)
----------------
I think we should avoid running the printer on every arg. Instead probably switch over the types of templatearguments, and delegate to shouldHintName(Expr, handle TagType/Typedef/other common nodes specifically, as done in shouldHintName).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138425