[clangd] Omit parameter hint if parameter name comment is present

[sammccall]

Just a few optional ideas, this is fine as-is

clang-tools-extra/clangd/InlayHints.cpp

113

nit: one of these trim()s can be an rtrim

126

In order to use sourcePrefix it we're doing the buffer lookup stuff for every param.

Would be a little more direct to grab the mainfile buffer + fileid at the start, and then here just decompose the top macrocallerloc straight away.

Seems a little simpler to reason about, no idea if the performance matters much, up to you.

135

I think we should handle the case where we never find /*. *maybe* it can't happen, but I wouldn't want to bet too much on it.

Alternatively, rewinding back to the start of the (possibly long) comment just to check if it matches a fixed string seems a little roundabout.

Could be:

if (!SourcePrefix.consume_back("*/")) return false;
SourcePrefix = SourcePrefix.rtrim().rtrim('=').rtrim();
if (!SourcePrefix.consume_back(ParamName)) return false;
return SourcePrefix.rtrim().endswith("/*");

[nridge]

Address review comments

clang-tools-extra/clangd/InlayHints.cpp

 #include "clang/Basic/SourceManager.h"
 
 namespace clang {
 namespace clangd {
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST)
-      : Results(Results), AST(AST.getASTContext()) {}
+      : Results(Results), AST(AST.getASTContext()),
+        MainFileID(AST.getSourceManager().getMainFileID()) {
+    bool Invalid = false;
+    llvm::StringRef Buf =
+        AST.getSourceManager().getBufferData(MainFileID, &Invalid);
+    MainFileBuf = Invalid ? StringRef{} : Buf;
+  }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // Weed out constructor calls that don't look like a function call with
     // an argument list, by checking the validity of getParenOrBraceRange().
     // Also weed out std::initializer_list constructors as there are no names
     // for the individual arguments.
     if (!E->getParenOrBraceRange().isValid() ||
         E->isStdInitListInitialization()) {
bool shouldHint(const Expr *Arg, StringRef ParamName) {
     if (ParamName.empty())
       return false;
 
     // If the argument expression is a single name and it matches the
     // parameter name exactly, omit the hint.
     if (ParamName == getSpelledIdentifier(Arg))
       return false;
 
-    // FIXME: Exclude argument expressions preceded by a /*paramName=*/ comment.
+    // Exclude argument expressions preceded by a /*paramName*/.
+    if (isPrecededByParamNameComment(Arg, ParamName))
+      return false;
 
     return true;
   }
 
+  // Checks if "E" is spelled in the main file and preceded by a C-style comment
+  // whose contents match ParamName (allowing for whitespace and an optional "="
+  // at the end.
+  bool isPrecededByParamNameComment(const Expr *E, StringRef ParamName) {
+    auto &SM = AST.getSourceManager();
+    auto ExprStartLoc = SM.getTopMacroCallerLoc(E->getBeginLoc());
+    auto Decomposed = SM.getDecomposedLoc(ExprStartLoc);
+    if (Decomposed.first != MainFileID)
+      return false;
+
+    StringRef SourcePrefix = MainFileBuf.substr(0, Decomposed.second);
+    // Allow whitespace between comment and expression.
+    SourcePrefix = SourcePrefix.rtrim();
+    // Check for comment ending.
+    if (!SourcePrefix.consume_back("*/"))
+      return false;
+    // Allow whitespace and "=" at end of comment.
+    SourcePrefix = SourcePrefix.rtrim().rtrim('=').rtrim();
+    // Other than that, the comment must contain exactly ParamName.
+    if (!SourcePrefix.consume_back(ParamName))
+      return false;
+    return SourcePrefix.rtrim().endswith("/*");
+  }
+
   // If "E" spells a single unqualified identifier, return that name.
   // Otherwise, return an empty string.
   static StringRef getSpelledIdentifier(const Expr *E) {
     E = E->IgnoreUnlessSpelledInSource();
 
     if (auto *DRE = dyn_cast<DeclRefExpr>(E))
       if (!DRE->getQualifier())
         return getSimpleName(*DRE->getDecl());
Results.push_back(InlayHint{
         Range{
             sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
             sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())},
         Kind, Label.str()});
   }
 
   std::vector<InlayHint> &Results;
   ASTContext &AST;
+  FileID MainFileID;
+  StringRef MainFileBuf;
 };
 
 std::vector<InlayHint> inlayHints(ParsedAST &AST) {
   std::vector<InlayHint> Results;
   InlayHintVisitor Visitor(Results, AST);
   Visitor.TraverseAST(AST.getASTContext());
   return Results;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

TEST(ParameterHints, UserDefinedLiteral) {
   assertParameterHints(R"cpp(
     long double operator"" _w(long double param);
     void bar() {
       1.2_w;
     }
   )cpp");
 }
 
+TEST(ParameterHints, ParamNameComment) {
+  // Do not hint an argument which already has a comment
+  // with the parameter name preceding it.
+  assertParameterHints(R"cpp(
+    void foo(int param);
+    void bar() {
+      foo(/*param*/42);
+      foo( /* param = */ 42);
+      foo(/* the answer */$param[[42]]);
+    }
+  )cpp",
+                       ExpectedHint{"param: ", "param"});
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang