[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

108

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

122

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.

131

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

 //===--- InlayHints.cpp ------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "InlayHints.h"
 #include "ParsedAST.h"
+#include "SourceCode.h"
 #include "support/Logger.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
 
 namespace clang {
 namespace clangd {
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*/.
+    // Allow the comment to contain spaces and optionally an "=" at the end.
+    if (getPrecedingComment(Arg).trim().rtrim("=").trim() == ParamName)
+      return false;
 
     return true;
   }
 
+  // If "E" is spelled in the main file and is preceded by a C-style comment,
+  // return the contents of that comment.
+  StringRef getPrecedingComment(const Expr *E) {
+    auto &SM = AST.getSourceManager();
+    auto ExprStartLoc = SM.getTopMacroCallerLoc(E->getBeginLoc());
+    if (!ExprStartLoc.isFileID())
+      return {};
+
+    StringRef SourcePrefix = sourcePrefix(ExprStartLoc, SM);
+    // Trim whitespace preceding expression.
+    SourcePrefix = SourcePrefix.rtrim();
+    // Preceding token is not a C-style comment.
+    if (!SourcePrefix.endswith("*/"))
+      return {};
+    // Trim end of comment.
+    SourcePrefix = SourcePrefix.substr(0, SourcePrefix.size() - 2);
+    // Find start of comment and get contents.
+    return SourcePrefix.substr(SourcePrefix.rfind("/*") + 2);
+  }
+
   // 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());

clang-tools-extra/clangd/SourceCode.h

 /// Infers whether this is a header from the FileName and LangOpts (if
 /// presents).
 bool isHeaderFile(llvm::StringRef FileName,
                   llvm::Optional<LangOptions> LangOpts = llvm::None);
 
 /// Returns true if the given location is in a generated protobuf file.
 bool isProtoFile(SourceLocation Loc, const SourceManager &SourceMgr);
 
+// Return the source code of the file containing Loc, from the start up to Loc.
+llvm::StringRef sourcePrefix(SourceLocation Loc, const SourceManager &SM);
+
 } // namespace clangd
 } // namespace clang
 #endif

clang-tools-extra/clangd/SourceCode.cpp

bool isProtoFile(SourceLocation Loc, const SourceManager &SM) {
   auto FID = SM.getFileID(Loc);
   // All proto generated headers should start with this line.
   static const char *PROTO_HEADER_COMMENT =
       "// Generated by the protocol buffer compiler.  DO NOT EDIT!";
   // Double check that this is an actual protobuf header.
   return SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT);
 }
 
+llvm::StringRef sourcePrefix(SourceLocation Loc, const SourceManager &SM) {
+  auto D = SM.getDecomposedLoc(Loc);
+  bool Invalid = false;
+  llvm::StringRef Buf = SM.getBufferData(D.first, &Invalid);
+  if (Invalid || D.second > Buf.size())
+    return "";
+  return Buf.substr(0, D.second);
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/XRefs.cpp

 }
 
 bool tokenSpelledAt(SourceLocation SpellingLoc, const syntax::TokenBuffer &TB) {
   auto ExpandedTokens = TB.expandedTokens(
       TB.sourceManager().getMacroArgExpandedLocation(SpellingLoc));
   return !ExpandedTokens.empty();
 }
 
-llvm::StringRef sourcePrefix(SourceLocation Loc, const SourceManager &SM) {
-  auto D = SM.getDecomposedLoc(Loc);
-  bool Invalid = false;
-  llvm::StringRef Buf = SM.getBufferData(D.first, &Invalid);
-  if (Invalid || D.second > Buf.size())
-    return "";
-  return Buf.substr(0, D.second);
-}
-
 bool isDependentName(ASTNodeKind NodeKind) {
   return NodeKind.isSame(ASTNodeKind::getFromNodeKind<OverloadExpr>()) ||
          NodeKind.isSame(
              ASTNodeKind::getFromNodeKind<CXXDependentScopeMemberExpr>()) ||
          NodeKind.isSame(
              ASTNodeKind::getFromNodeKind<DependentScopeDeclRefExpr>());
 }
 
findExplicitReferences(
         }
       },
       AST.getHeuristicResolver());
   return DeclRefs;
 }
 
 } // namespace clangd
 } // namespace clang
 No newline at end of file

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