Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 31 additions & 13 deletions clang-tools-extra/clangd/CodeComplete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "AST.h"
#include "CodeCompletionStrings.h"
#include "Compiler.h"
#include "Config.h"
#include "ExpectedTypes.h"
#include "Feature.h"
#include "FileDistance.h"
Expand Down Expand Up @@ -350,8 +351,7 @@ struct CodeCompletionBuilder {
CodeCompletionContext::Kind ContextKind,
const CodeCompleteOptions &Opts,
bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
: ASTCtx(ASTCtx),
EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
: ASTCtx(ASTCtx), ArgumentLists(Opts.ArgumentLists),
IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
Completion.Deprecated = true; // cleared by any non-deprecated overload.
add(C, SemaCCS, ContextKind);
Expand Down Expand Up @@ -561,14 +561,32 @@ struct CodeCompletionBuilder {
}

std::string summarizeSnippet() const {

/// localize a ArgList depending on the config. If the config is unset we
/// will use our local (this) version, else use the one of the config
const Config::ArgumentListsOption ArgList =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with our existing Completion option (AllScopes), let's do the Config lookup here and propagate it into CodeCompleteOpts.

That then gets propagated to CodeCompletionBuilder::ArgumentLists in the CodeCompletionBuilder constructor, and here we can just use the ArgumentLists member without doing anything further.

Config::current().Completion.ArgumentLists !=
Config::ArgumentListsOption::UnsetDefault
? Config::current().Completion.ArgumentLists
: ArgumentLists;

/// localize ArgList value, tests for better readability
const bool None = ArgList == Config::ArgumentListsOption::None;
const bool Open = ArgList == Config::ArgumentListsOption::OpenDelimiter;
const bool Delim = ArgList == Config::ArgumentListsOption::Delimiters;
const bool Full =
ArgList == Config::ArgumentListsOption::FullPlaceholders ||
ArgList == Config::ArgumentListsOption::UnsetDefault ||
(!None && !Open && !Delim); // <-- failsafe

if (IsUsingDeclaration)
return "";
auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
if (!Snippet)
// All bundles are function calls.
// FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
// we need to complete 'forward<$1>($0)'.
return "($0)";
return None ? "" : (Open ? "(" : "($0)");

if (Snippet->empty())
return "";
Expand Down Expand Up @@ -607,7 +625,7 @@ struct CodeCompletionBuilder {
return "";
}
}
if (EnableFunctionArgSnippets)
if (Full)
return *Snippet;

// Replace argument snippets with a simplified pattern.
Expand All @@ -622,9 +640,9 @@ struct CodeCompletionBuilder {

bool EmptyArgs = llvm::StringRef(*Snippet).ends_with("()");
if (Snippet->front() == '<')
return EmptyArgs ? "<$1>()$0" : "<$1>($0)";
return None ? "" : (Open ? "<" : (EmptyArgs ? "<$1>()$0" : "<$1>($0)"));
if (Snippet->front() == '(')
return EmptyArgs ? "()" : "($0)";
return None ? "" : (Open ? "(" : (EmptyArgs ? "()" : "($0)"));
return *Snippet; // Not an arg snippet?
}
// 'CompletionItemKind::Interface' matches template type aliases.
Expand All @@ -638,7 +656,7 @@ struct CodeCompletionBuilder {
// e.g. Foo<${1:class}>.
if (llvm::StringRef(*Snippet).ends_with("<>"))
return "<>"; // can happen with defaulted template arguments.
return "<$0>";
return None ? "" : (Open ? "<" : "<$0>");
}
return *Snippet;
}
Expand All @@ -654,7 +672,8 @@ struct CodeCompletionBuilder {
ASTContext *ASTCtx;
CodeCompletion Completion;
llvm::SmallVector<BundledEntry, 1> Bundled;
bool EnableFunctionArgSnippets;
/// the way argument lists are handled.
Config::ArgumentListsOption ArgumentLists;
// No snippets will be generated for using declarations and when the function
// arguments are already present.
bool IsUsingDeclaration;
Expand Down Expand Up @@ -797,8 +816,8 @@ SpecifiedScope getQueryScopes(CodeCompletionContext &CCContext,
llvm::StringRef SpelledSpecifier = Lexer::getSourceText(
CharSourceRange::getCharRange(SemaSpecifier->getRange()),
CCSema.SourceMgr, clang::LangOptions());
if (SpelledSpecifier.consume_front("::"))
Scopes.QueryScopes = {""};
if (SpelledSpecifier.consume_front("::"))
Scopes.QueryScopes = {""};
Scopes.UnresolvedQualifier = std::string(SpelledSpecifier);
// Sema excludes the trailing "::".
if (!Scopes.UnresolvedQualifier->empty())
Expand Down Expand Up @@ -1591,7 +1610,7 @@ class CodeCompleteFlow {
CompletionPrefix HeuristicPrefix;
std::optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
Range ReplacedRange;
std::vector<std::string> QueryScopes; // Initialized once Sema runs.
std::vector<std::string> QueryScopes; // Initialized once Sema runs.
std::vector<std::string> AccessibleScopes; // Initialized once Sema runs.
// Initialized once QueryScopes is initialized, if there are scopes.
std::optional<ScopeDistance> ScopeProximity;
Expand Down Expand Up @@ -2387,8 +2406,7 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const CodeCompletion &C) {
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const CodeCompleteResult &R) {
OS << "CodeCompleteResult: " << R.Completions.size() << (R.HasMore ? "+" : "")
<< " (" << getCompletionKindString(R.Context) << ")"
<< " items:\n";
<< " (" << getCompletionKindString(R.Context) << ")" << " items:\n";
for (const auto &C : R.Completions)
OS << C << "\n";
return OS;
Expand Down
9 changes: 5 additions & 4 deletions clang-tools-extra/clangd/CodeComplete.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "ASTSignals.h"
#include "Compiler.h"
#include "Config.h"
#include "Protocol.h"
#include "Quality.h"
#include "index/Index.h"
Expand Down Expand Up @@ -96,17 +97,17 @@ struct CodeCompleteOptions {
/// '->' on member access etc.
bool IncludeFixIts = false;

/// Whether to generate snippets for function arguments on code-completion.
/// Needs snippets to be enabled as well.
bool EnableFunctionArgSnippets = true;

/// Whether to include index symbols that are not defined in the scopes
/// visible from the code completion point. This applies in contexts without
/// explicit scope qualifiers.
///
/// Such completions can insert scope qualifiers.
bool AllScopes = false;

/// The way argument list on calls '()' and generics '<>' are handled.
Config::ArgumentListsOption ArgumentLists =
Config::ArgumentListsOption::UnsetDefault;

/// Whether to use the clang parser, or fallback to text-based completion
/// (using identifiers in the current file and symbol indexes).
enum CodeCompletionParse {
Expand Down
19 changes: 19 additions & 0 deletions clang-tools-extra/clangd/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,29 @@ struct Config {
std::vector<std::string> FullyQualifiedNamespaces;
} Style;

/// controls the completion options for argument lists.
enum class ArgumentListsOption {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming suggestion: ArgumentListsPolicy, for consistency with other enums defined in this file

/// the default value. This will imply FullPlaceholders unless overridden by
/// --function-arg-placeholders=0, in which case Delimiters is used.
/// any other use-case of --function-arg-placeholders is ignored
UnsetDefault = 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value isn't necessary.

The way config providers interact, using BackgroundPolicy (an existing config option which also exists as a command-line flag) as an example:

  1. A Config object is created
  2. FlagsConfigProvider runs first. If the relevant command-line flag (--background-index) was specified, it assigns the provided value to the Index.Background field of the Config object created at step (1). Note, if the flag wasn't provided, this line doesn't run at all.
  3. Then the config provider for the config file runs. The data structure representing data parsed from the file stores everything using std::optional, which is only populated if the key is present in the config file. That then overwrites the Index.Background field of the same Config object, but once again this line only runs at all if the optional was populated.

As a result, no "unset" value is needed; std::optional takes care of that. The default value of Completion::ArgumentLists below -- used if neither a command-line flag nor a config file key was specified -- should be FullPlaceholders.

/// nothing, no argument list and also NO Delimiters "()" or "<>"
None,
/// open, only opening delimiter "(" or "<"
OpenDelimiter,
/// empty pair of delimiters "()" or "<>" (or [legacy] alias 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these mentions of legacy aliases can be removed (it's just confusing since that applies to the command line flag which is defined elsewhere)

Delimiters,
/// full name of both type and variable (or [legacy] alias 1)
FullPlaceholders,
};

/// Configures code completion feature.
struct {
/// Whether code completion includes results that are not visible in current
/// scopes.
bool AllScopes = true;
/// controls the completion options for argument lists.
ArgumentListsOption ArgumentLists = ArgumentListsOption::UnsetDefault;
} Completion;

/// Configures hover feature.
Expand All @@ -150,6 +168,7 @@ struct Config {
bool BlockEnd = false;
// Limit the length of type names in inlay hints. (0 means no limit)
uint32_t TypeNameLimit = 32;

} InlayHints;

struct {
Expand Down
33 changes: 29 additions & 4 deletions clang-tools-extra/clangd/ConfigCompile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -504,10 +504,10 @@ struct FragmentCompiler {
auto Fast = isFastTidyCheck(Str);
if (!Fast.has_value()) {
diag(Warning,
llvm::formatv(
"Latency of clang-tidy check '{0}' is not known. "
"It will only run if ClangTidy.FastCheckFilter is Loose or None",
Str)
llvm::formatv("Latency of clang-tidy check '{0}' is not known. "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm seeing some diff hunks containing unrelated formatting changes. If would be great to remove this as they pollute the blame

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use clang-format with the .clang-format of the code base. Even if I don't change anything and just do a clang-format this change will happen. It seems that it's not correctly formatted. I will fix this and my recommendation would be to use clang-format on this file - or perhaps the whole code base - post patching in its own commit. this will fix this issue(s) in the future.

"It will only run if ClangTidy.FastCheckFilter is "
"Loose or None",
Str)
.str(),
Arg.Range);
} else if (!*Fast) {
Expand Down Expand Up @@ -622,6 +622,31 @@ struct FragmentCompiler {
C.Completion.AllScopes = AllScopes;
});
}
if (F.ArgumentLists) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a compileEnum helper to do this more easily, see the handling of BackgroundPolicy as an example

if (**F.ArgumentLists == "None") {
Out.Apply.push_back(
[ArgumentLists(**F.ArgumentLists)](const Params &, Config &C) {
C.Completion.ArgumentLists = Config::ArgumentListsOption::None;
});
} else if (**F.ArgumentLists == "OpenDelimiter") {
Out.Apply.push_back(
[ArgumentLists(**F.ArgumentLists)](const Params &, Config &C) {
C.Completion.ArgumentLists =
Config::ArgumentListsOption::OpenDelimiter;
});
} else if (**F.ArgumentLists == "Delimiters") {
Out.Apply.push_back([ArgumentLists(**F.ArgumentLists)](const Params &,
Config &C) {
C.Completion.ArgumentLists = Config::ArgumentListsOption::Delimiters;
});
} else if (**F.ArgumentLists == "FullPlaceholders") {
Out.Apply.push_back(
[ArgumentLists(**F.ArgumentLists)](const Params &, Config &C) {
C.Completion.ArgumentLists =
Config::ArgumentListsOption::FullPlaceholders;
});
}
}
}

void compile(Fragment::HoverBlock &&F) {
Expand Down
8 changes: 8 additions & 0 deletions clang-tools-extra/clangd/ConfigFragment.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGFRAGMENT_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGFRAGMENT_H

#include "Config.h"
#include "ConfigProvider.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/SourceMgr.h"
Expand Down Expand Up @@ -308,6 +309,13 @@ struct Fragment {
/// Whether code completion should include suggestions from scopes that are
/// not visible. The required scope prefix will be inserted.
std::optional<Located<bool>> AllScopes;
/// How to present the argument list between '()' and '<>':
/// valid values are enum Config::ArgumentListsOption values:
/// None: Nothing at all
/// OpenDelimiter: only opening delimiter "(" or "<"
/// Delimiters: empty pair of delimiters "()" or "<>"
/// FullPlaceholders: full name of both type and variable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: variable --> parameter

std::optional<Located<std::string>> ArgumentLists;
};
CompletionBlock Completion;

Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/clangd/ConfigYAML.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ class Parser {
if (auto AllScopes = boolValue(N, "AllScopes"))
F.AllScopes = *AllScopes;
});
Dict.handle("ArgumentLists", [&](Node &N) {
if (auto ArgumentLists = scalarValue(N, "ArgumentLists"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since F.ArgumentLists is itself an optional, this can be simplified to:

F.ArgumentLists = scalarValue(N, "ArgumentLists");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this comment remains to be addressed)

F.ArgumentLists = *ArgumentLists;
});
Dict.parse(N);
}

Expand Down
26 changes: 18 additions & 8 deletions clang-tools-extra/clangd/tool/ClangdMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,13 +242,16 @@ opt<std::string> FallbackStyle{
init(clang::format::DefaultFallbackStyle),
};

opt<bool> EnableFunctionArgSnippets{
"function-arg-placeholders",
cat(Features),
desc("When disabled, completions contain only parentheses for "
"function calls. When enabled, completions also contain "
"placeholders for method parameters"),
init(CodeCompleteOptions().EnableFunctionArgSnippets),
opt<Config::ArgumentListsOption> PlaceholderOption{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity and consistency with --background-index, let's keep the name and type of this unchanged

"function-arg-placeholders", cat(Features),
desc("Set the way placeholders and delimiters are implemented."),
init(CodeCompleteOptions().ArgumentLists),
values(clEnumValN(Config::ArgumentListsOption::Delimiters, "0",
"Empty pair of delimiters inserted"),
clEnumValN(
Config::ArgumentListsOption::FullPlaceholders, "1",
"[default] Delimiters and placeholder arguments are inserted."))

};

opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion{
Expand Down Expand Up @@ -650,6 +653,7 @@ class FlagsConfigProvider : public config::Provider {
std::optional<Config::CDBSearchSpec> CDBSearch;
std::optional<Config::ExternalIndexSpec> IndexSpec;
std::optional<Config::BackgroundPolicy> BGPolicy;
std::optional<Config::ArgumentListsOption> ArgumentLists;

// If --compile-commands-dir arg was invoked, check value and override
// default path.
Expand Down Expand Up @@ -694,13 +698,20 @@ class FlagsConfigProvider : public config::Provider {
BGPolicy = Config::BackgroundPolicy::Skip;
}

if (PlaceholderOption == Config::ArgumentListsOption::Delimiters) {
ArgumentLists = PlaceholderOption;
}

Frag = [=](const config::Params &, Config &C) {
if (CDBSearch)
C.CompileFlags.CDBSearch = *CDBSearch;
if (IndexSpec)
C.Index.External = *IndexSpec;
if (BGPolicy)
C.Index.Background = *BGPolicy;
if (ArgumentLists && C.Completion.ArgumentLists ==
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reasons mentioned above, no need for the second condition here (at this point, the config provider for the config file has not yet assigned anything to C; when it does run later, it will overwrite the field if needed)

Config::ArgumentListsOption::UnsetDefault)
C.Completion.ArgumentLists = *ArgumentLists;
if (AllScopesCompletion.getNumOccurrences())
C.Completion.AllScopes = AllScopesCompletion;

Expand Down Expand Up @@ -916,7 +927,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
Opts.CodeComplete.IncludeIndicator.Insert.clear();
Opts.CodeComplete.IncludeIndicator.NoInsert.clear();
}
Opts.CodeComplete.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
Opts.CodeComplete.RunParser = CodeCompletionParse;
Opts.CodeComplete.RankingModel = RankingModel;

Expand Down
Loading