[clangd] Make EnableFunctionArgSnippets option string-typed

[HighCommander4]

Fixes clangd/clangd#2232

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Nathan Ridge (HighCommander4)

Changes

Fixes clangd/clangd#2232

---

Full diff: https://github.com/llvm/llvm-project/pull/121178.diff

1 Files Affected:

• (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (+21-6)

diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index cc061e2d932314..d89b4d81d830d0 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -242,13 +242,13 @@ opt<std::string> FallbackStyle{
     init(clang::format::DefaultFallbackStyle),
 };
 
-opt<int> EnableFunctionArgSnippets{
+opt<std::string> EnableFunctionArgSnippets{
     "function-arg-placeholders",
     cat(Features),
     desc("When disabled (0), completions contain only parentheses for "
          "function calls. When enabled (1), completions also contain "
          "placeholders for method parameters"),
-    init(-1),
+    init("-1"),
 };
 
 opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
@@ -635,6 +635,22 @@ loadExternalIndex(const Config::ExternalIndexSpec &External,
   llvm_unreachable("Invalid ExternalIndexKind.");
 }
 
+std::optional<bool> shouldEnableFunctionArgSnippets() {
+  std::string Val = EnableFunctionArgSnippets;
+  // Accept the same values that a bool option parser would, but also accept
+  // -1 to indicate "unspecified", in which case the ArgumentListsPolicy
+  // config option will be respected.
+  if (Val == "1" || Val == "true" || Val == "True" || Val == "TRUE")
+    return true;
+  if (Val == "0" || Val == "false" || Val == "False" || Val == "FALSE")
+    return false;
+  if (Val != "-1")
+    elog("Value specified by --function-arg-placeholders is invalid. Provide a "
+         "boolean value or leave unspecified to use ArgumentListsPolicy from "
+         "config instead.");
+  return std::nullopt;
+}
+
 class FlagsConfigProvider : public config::Provider {
 private:
   config::CompiledFragment Frag;
@@ -695,10 +711,9 @@ class FlagsConfigProvider : public config::Provider {
       BGPolicy = Config::BackgroundPolicy::Skip;
     }
 
-    if (EnableFunctionArgSnippets >= 0) {
-      ArgumentLists = EnableFunctionArgSnippets
-                          ? Config::ArgumentListsPolicy::FullPlaceholders
-                          : Config::ArgumentListsPolicy::Delimiters;
+    if (std::optional<bool> Enable = shouldEnableFunctionArgSnippets()) {
+      ArgumentLists = *Enable ? Config::ArgumentListsPolicy::FullPlaceholders
+                              : Config::ArgumentListsPolicy::Delimiters;
     }
 
     Frag = [=](const config::Params &, Config &C) {

[llvmbot]

@llvm/pr-subscribers-clangd

Author: Nathan Ridge (HighCommander4)

Changes

Fixes clangd/clangd#2232

---

Full diff: https://github.com/llvm/llvm-project/pull/121178.diff

1 Files Affected:

• (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (+21-6)

diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index cc061e2d932314..d89b4d81d830d0 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -242,13 +242,13 @@ opt<std::string> FallbackStyle{
     init(clang::format::DefaultFallbackStyle),
 };
 
-opt<int> EnableFunctionArgSnippets{
+opt<std::string> EnableFunctionArgSnippets{
     "function-arg-placeholders",
     cat(Features),
     desc("When disabled (0), completions contain only parentheses for "
          "function calls. When enabled (1), completions also contain "
          "placeholders for method parameters"),
-    init(-1),
+    init("-1"),
 };
 
 opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
@@ -635,6 +635,22 @@ loadExternalIndex(const Config::ExternalIndexSpec &External,
   llvm_unreachable("Invalid ExternalIndexKind.");
 }
 
+std::optional<bool> shouldEnableFunctionArgSnippets() {
+  std::string Val = EnableFunctionArgSnippets;
+  // Accept the same values that a bool option parser would, but also accept
+  // -1 to indicate "unspecified", in which case the ArgumentListsPolicy
+  // config option will be respected.
+  if (Val == "1" || Val == "true" || Val == "True" || Val == "TRUE")
+    return true;
+  if (Val == "0" || Val == "false" || Val == "False" || Val == "FALSE")
+    return false;
+  if (Val != "-1")
+    elog("Value specified by --function-arg-placeholders is invalid. Provide a "
+         "boolean value or leave unspecified to use ArgumentListsPolicy from "
+         "config instead.");
+  return std::nullopt;
+}
+
 class FlagsConfigProvider : public config::Provider {
 private:
   config::CompiledFragment Frag;
@@ -695,10 +711,9 @@ class FlagsConfigProvider : public config::Provider {
       BGPolicy = Config::BackgroundPolicy::Skip;
     }
 
-    if (EnableFunctionArgSnippets >= 0) {
-      ArgumentLists = EnableFunctionArgSnippets
-                          ? Config::ArgumentListsPolicy::FullPlaceholders
-                          : Config::ArgumentListsPolicy::Delimiters;
+    if (std::optional<bool> Enable = shouldEnableFunctionArgSnippets()) {
+      ArgumentLists = *Enable ? Config::ArgumentListsPolicy::FullPlaceholders
+                              : Config::ArgumentListsPolicy::Delimiters;
     }
 
     Frag = [=](const config::Params &, Config &C) {

[HighCommander4]

The patch doesn't include a test because the only way I can think of testing this is a clangd lit test and it doesn't seem worth writing one for this, but I could write one if desired.

[hokein]

Not a very related comment, I think we need to add the new ArgumentLists config in the clangd config doc https://clangd.llvm.org/config#completion.

[hokein]

We could also consider the following option, but up to you.

enum FunctionArgSnippetsFlag { True, False, None, ... };
opt<FunctionArgSnippetsFlag> EnableFunctionArgSnippets{...
  values(
    clEnumValN(FunctionArgSnippetsFlag::True, "true", ...),
    clEnumValN(FunctionArgSnippetsFlag::True, "True", ...),
    ...
  ),
};
  

[HighCommander4]

Thanks for the suggestion. I tried this briefly, but I think it makes the --help output too noisy because it prints a separate line for each spelling of "true", "True", etc.

[HighCommander4]

I think we need to add the new ArgumentLists config in the clangd config doc https://clangd.llvm.org/config#completion.

Definitely. I'll send out a patch documenting this and other options new in clangd 20 in the near future. (We have an entry about this on the Release checklist now, so we don't forget.)