[clang-tools-extra] 6f6cf2d - [clangd] Omit parameter hint for setter functions

[Nathan Ridge via cfe-commits]

Author: Nathan Ridge
Date: 2021-04-25T19:20:12-04:00
New Revision: 6f6cf2da8d9453f2bd57978be1e5d0765ea50a36

URL: https://github.com/llvm/llvm-project/commit/6f6cf2da8d9453f2bd57978be1e5d0765ea50a36
DIFF: https://github.com/llvm/llvm-project/commit/6f6cf2da8d9453f2bd57978be1e5d0765ea50a36.diff

LOG: [clangd] Omit parameter hint for setter functions

Differential Revision: https://reviews.llvm.org/D100731

Added: 
    

Modified: 
    clang-tools-extra/clangd/InlayHints.cpp
    clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 948a9e511e7f..a9f3100a187e 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -59,6 +59,8 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
+  using NameVec = SmallVector<StringRef, 8>;
+
   // The purpose of Anchor is to deal with macros. It should be the call's
   // opening or closing parenthesis or brace. (Always using the opening would
   // make more sense but CallExpr only exposes the closing.) We heuristically
@@ -79,16 +81,17 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
       if (Ctor->isCopyOrMoveConstructor())
         return;
 
-    // FIXME: Exclude setters (i.e. functions with one argument whose name
-    // begins with "set"), as their parameter name is also not likely to be
-    // interesting.
-
     // Don't show hints for variadic parameters.
     size_t FixedParamCount = getFixedParamCount(Callee);
     size_t ArgCount = std::min(FixedParamCount, Args.size());
 
     NameVec ParameterNames = chooseParameterNames(Callee, ArgCount);
 
+    // Exclude setters (i.e. functions with one argument whose name begins with
+    // "set"), as their parameter name is also not likely to be interesting.
+    if (isSetter(Callee, ParameterNames))
+      return;
+
     for (size_t I = 0; I < ArgCount; ++I) {
       StringRef Name = ParameterNames[I];
       if (!shouldHint(Args[I], Name))
@@ -99,6 +102,29 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
     }
   }
 
+  static bool isSetter(const FunctionDecl *Callee, const NameVec &ParamNames) {
+    if (ParamNames.size() != 1)
+      return false;
+
+    StringRef Name = getSimpleName(*Callee);
+    if (!Name.startswith_lower("set"))
+      return false;
+
+    // In addition to checking that the function has one parameter and its
+    // name starts with "set", also check that the part after "set" matches
+    // the name of the parameter (ignoring case). The idea here is that if
+    // the parameter name 
diff ers, it may contain extra information that
+    // may be useful to show in a hint, as in:
+    //   void setTimeout(int timeoutMillis);
+    // This currently doesn't handle cases where params use snake_case
+    // and functions don't, e.g.
+    //   void setExceptionHandler(EHFunc exception_handler);
+    // We could improve this by replacing `equals_lower` with some
+    // `sloppy_equals` which ignores case and also skips underscores.
+    StringRef WhatItIsSetting = Name.substr(3).ltrim("_");
+    return WhatItIsSetting.equals_lower(ParamNames[0]);
+  }
+
   bool shouldHint(const Expr *Arg, StringRef ParamName) {
     if (ParamName.empty())
       return false;
@@ -155,8 +181,6 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
     return {};
   }
 
-  using NameVec = SmallVector<StringRef, 8>;
-
   NameVec chooseParameterNames(const FunctionDecl *Callee, size_t ArgCount) {
     // The current strategy here is to use all the parameter names from the
     // canonical declaration, unless they're all empty, in which case we

diff  --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 086c502181ef..23f0e9ef11d3 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -336,6 +336,30 @@ TEST(ParameterHints, ParamNameComment) {
                        ExpectedHint{"param: ", "param"});
 }
 
+TEST(ParameterHints, SetterFunctions) {
+  assertParameterHints(R"cpp(
+    struct S {
+      void setParent(S* parent);
+      void set_parent(S* parent);
+      void setTimeout(int timeoutMillis);
+      void setTimeoutMillis(int timeout_millis);
+    };
+    void bar() {
+      S s;
+      // Parameter name matches setter name - omit hint.
+      s.setParent(nullptr);
+      // Support snake_case
+      s.set_parent(nullptr);
+      // Parameter name may contain extra info - show hint.
+      s.setTimeout($timeoutMillis[[120]]);
+      // FIXME: Ideally we'd want to omit this.
+      s.setTimeoutMillis($timeout_millis[[120]]);
+    }
+  )cpp",
+                       ExpectedHint{"timeoutMillis: ", "timeoutMillis"},
+                       ExpectedHint{"timeout_millis: ", "timeout_millis"});
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang