nridge updated this revision to Diff 340392.
nridge marked 2 inline comments as done.
nridge added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100731/new/
https://reviews.llvm.org/D100731
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -336,6 +336,30 @@
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
Index: clang-tools-extra/clangd/InlayHints.cpp
===================================================================
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -59,6 +59,8 @@
// 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 @@
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 @@
}
}
+ 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 differs, 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 @@
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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
