[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.
sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:762 Opts.ResourceDir = ResourceDir; - Opts.BuildDynamicSymbolIndex = EnableIndex; + Opts.BuildDynamicSymbolIndex = true; Opts.CollectMainFileRefs = CollectMainFileRefs; ivanmurashko wrote: > @sammccall The option is always true, do we need it as an option? It's false in tests, except when we're testing the index specifically. On balance I think this is a good think because it allows us to reason about a simpler system when designing/debugging tests. In particular we don't have to worry about the AST-paths in tests "cheating" by looking at the index. Seems sensible to make the struct default true and set it to false explicitly in optsForTest, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94727/new/ https://reviews.llvm.org/D94727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.
ivanmurashko added inline comments. Herald added projects: clang-tools-extra, All. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:762 Opts.ResourceDir = ResourceDir; - Opts.BuildDynamicSymbolIndex = EnableIndex; + Opts.BuildDynamicSymbolIndex = true; Opts.CollectMainFileRefs = CollectMainFileRefs; @sammccall The option is always true, do we need it as an option? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94727/new/ https://reviews.llvm.org/D94727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2ab5fd2c8567: [clangd] Retire some flags for
uncontroversial, stable features. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D94727?vs=316787=317819#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94727/new/
https://reviews.llvm.org/D94727
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Compiler.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/TestTU.cpp
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -62,8 +62,6 @@
if (ClangTidyProvider)
Inputs.ClangTidyProvider = ClangTidyProvider;
Inputs.Index = ExternalIndex;
- if (Inputs.Index)
-Inputs.Opts.SuggestMissingIncludes = true;
return Inputs;
}
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -80,8 +80,21 @@
OptionCategory Features("clangd feature options");
OptionCategory Misc("clangd miscellaneous options");
OptionCategory Protocol("clangd protocol and logging options");
+OptionCategory Retired("clangd flags no longer in use");
const OptionCategory *ClangdCategories[] = {, ,
-, };
+, , };
+
+template class RetiredFlag {
+ opt Option;
+
+public:
+ RetiredFlag(llvm::StringRef Name)
+ : Option(Name, cat(Retired), desc("Obsolete flag, ignored"), Hidden,
+ llvm::cl::callback([Name](const T &) {
+ llvm::errs()
+ << "The flag `-" << Name << "` is obsolete and ignored.\n";
+ })) {}
+};
enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
opt CompileArgsFrom{
@@ -267,15 +280,7 @@
Hidden,
};
-opt EnableIndex{
-"index",
-cat(Features),
-desc("Enable index-based features. By default, clangd maintains an index "
- "built from symbols in opened files. Global index support needs to "
- "enabled separatedly"),
-init(true),
-Hidden,
-};
+RetiredFlag EnableIndex("index");
opt LimitResults{
"limit-results",
@@ -285,13 +290,7 @@
init(100),
};
-opt SuggestMissingIncludes{
-"suggest-missing-includes",
-cat(Features),
-desc("Attempts to fix diagnostic errors caused by missing "
- "includes using index"),
-init(true),
-};
+RetiredFlag SuggestMissingIncludes("suggest-missing-includes");
list TweakList{
"tweaks",
@@ -308,21 +307,8 @@
init(true),
};
-opt RecoveryAST{
-"recovery-ast",
-cat(Features),
-desc("Preserve expressions in AST for broken code."),
-init(false),
-Hidden,
-};
-
-opt RecoveryASTType{
-"recovery-ast-type",
-cat(Features),
-desc("Preserve the type for recovery AST."),
-init(false),
-Hidden,
-};
+RetiredFlag RecoveryAST("recovery-ast");
+RetiredFlag RecoveryASTType("recovery-ast-type");
opt FoldingRanges{
"folding-ranges",
@@ -464,6 +450,7 @@
init(false),
};
+// FIXME: retire this flag in llvm 13 release cycle.
opt AsyncPreamble{
"async-preamble",
cat(Misc),
@@ -487,11 +474,13 @@
init(true),
};
+// FIXME: retire this flag in llvm 13 release cycle.
opt CollectMainFileRefs{
"collect-main-file-refs",
cat(Misc),
desc("Store references to main-file-only symbols in the index"),
init(ClangdServer::Options().CollectMainFileRefs),
+Hidden,
};
#if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
@@ -770,12 +759,12 @@
}
if (!ResourceDir.empty())
Opts.ResourceDir = ResourceDir;
- Opts.BuildDynamicSymbolIndex = EnableIndex;
+ Opts.BuildDynamicSymbolIndex = true;
Opts.CollectMainFileRefs = CollectMainFileRefs;
std::vector> IdxStack;
std::unique_ptr StaticIdx;
std::future AsyncIndexLoad; // Block exit while loading the index.
- if (EnableIndex && !IndexFile.empty()) {
+ if (!IndexFile.empty()) {
// Load the index asynchronously. Meanwhile SwapIndex returns no results.
SwapIndex *Placeholder;
StaticIdx.reset(Placeholder = new SwapIndex(std::make_unique()));
@@ -872,7 +861,6 @@
Opts.ClangTidyProvider = ClangTidyOptProvider;
}
Opts.AsyncPreambleBuilds = AsyncPreamble;
- Opts.SuggestMissingIncludes = SuggestMissingIncludes;
Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
Opts.TweakFilter = [&](const Tweak ) {
if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
---
[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.
sammccall added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:147 -bool SuggestMissingIncludes = false; - hokein wrote: > our internal client explicitly set this to `true`, so we need a migration > plan for this, otherwise this would break our build of internal client during > the integration, a possible plan is > > 1. set this flag to true by default in upstream, wait for the integration > 2. remove the explicit setting internally > 3. remove this flag (this patch) in upstream > > Or just remove the flag internally, then land this patch in upstream (but > internal release has to pick-up these two together) Discussed offline - there's no reason to block for out-of-tree clients here. (We're removing support for a configuration - SuggestMissingIncludes = false - which is AFAIK unused anywhere. And we're removing the flag as well, but this is a trivial API change to adapt to) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94727/new/ https://reviews.llvm.org/D94727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.
hokein added a comment. the code looks good to me, but we need to be a bit careful on landing this -- as we have an internal client setting this flag. Comment at: clang-tools-extra/clangd/ClangdServer.h:147 -bool SuggestMissingIncludes = false; - our internal client explicitly set this to `true`, so we need a migration plan for this, otherwise this would break our build of internal client during the integration, a possible plan is 1. set this flag to true by default in upstream, wait for the integration 2. remove the explicit setting internally 3. remove this flag (this patch) in upstream Or just remove the flag internally, then land this patch in upstream (but internal release has to pick-up these two together) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94727/new/ https://reviews.llvm.org/D94727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
And mark a couple to be retired afther the next release branch.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D94727
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Compiler.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/TestTU.cpp
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -62,8 +62,6 @@
if (ClangTidyProvider)
Inputs.ClangTidyProvider = ClangTidyProvider;
Inputs.Index = ExternalIndex;
- if (Inputs.Index)
-Inputs.Opts.SuggestMissingIncludes = true;
return Inputs;
}
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -80,8 +80,21 @@
OptionCategory Features("clangd feature options");
OptionCategory Misc("clangd miscellaneous options");
OptionCategory Protocol("clangd protocol and logging options");
+OptionCategory Retired("clangd flags no longer in use");
const OptionCategory *ClangdCategories[] = {, ,
-, };
+, , };
+
+template class RetiredFlag {
+ opt Option;
+
+public:
+ RetiredFlag(llvm::StringRef Name)
+ : Option(Name, cat(Retired), desc("Obsolete flag, ignored"), Hidden,
+ llvm::cl::callback([Name](const T &) {
+ llvm::errs()
+ << "The flag `-" << Name << "` is obsolete and ignored.\n";
+ })) {}
+};
enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
opt CompileArgsFrom{
@@ -267,15 +280,7 @@
Hidden,
};
-opt EnableIndex{
-"index",
-cat(Features),
-desc("Enable index-based features. By default, clangd maintains an index "
- "built from symbols in opened files. Global index support needs to "
- "enabled separatedly"),
-init(true),
-Hidden,
-};
+RetiredFlag EnableIndex("index");
opt LimitResults{
"limit-results",
@@ -285,13 +290,7 @@
init(100),
};
-opt SuggestMissingIncludes{
-"suggest-missing-includes",
-cat(Features),
-desc("Attempts to fix diagnostic errors caused by missing "
- "includes using index"),
-init(true),
-};
+RetiredFlag SuggestMissingIncludes("suggest-missing-includes");
list TweakList{
"tweaks",
@@ -308,20 +307,8 @@
init(true),
};
-opt RecoveryAST{
-"recovery-ast",
-cat(Features),
-desc("Preserve expressions in AST for broken code."),
-init(ClangdServer::Options().BuildRecoveryAST),
-};
-
-opt RecoveryASTType{
-"recovery-ast-type",
-cat(Features),
-desc("Preserve the type for recovery AST."),
-init(ClangdServer::Options().PreserveRecoveryASTType),
-Hidden,
-};
+RetiredFlag RecoveryAST("recovery-ast");
+RetiredFlag RecoveryASTType("recovery-ast-type");
opt FoldingRanges{
"folding-ranges",
@@ -463,6 +450,7 @@
init(false),
};
+// FIXME: retire this flag in llvm 13 release cycle.
opt AsyncPreamble{
"async-preamble",
cat(Misc),
@@ -486,11 +474,13 @@
init(true),
};
+// FIXME: retire this flag in llvm 13 release cycle.
opt CollectMainFileRefs{
"collect-main-file-refs",
cat(Misc),
desc("Store references to main-file-only symbols in the index"),
init(ClangdServer::Options().CollectMainFileRefs),
+Hidden,
};
#if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
@@ -769,12 +759,12 @@
}
if (!ResourceDir.empty())
Opts.ResourceDir = ResourceDir;
- Opts.BuildDynamicSymbolIndex = EnableIndex;
+ Opts.BuildDynamicSymbolIndex = true;
Opts.CollectMainFileRefs = CollectMainFileRefs;
std::vector> IdxStack;
std::unique_ptr StaticIdx;
std::future AsyncIndexLoad; // Block exit while loading the index.
- if (EnableIndex && !IndexFile.empty()) {
+ if (!IndexFile.empty()) {
// Load the index asynchronously. Meanwhile SwapIndex returns no results.
SwapIndex *Placeholder;
StaticIdx.reset(Placeholder = new SwapIndex(std::make_unique()));
@@ -813,8 +803,6 @@
Opts.StaticIndex = PAI.get();
}
Opts.AsyncThreadsCount = WorkerThreadsCount;
- Opts.BuildRecoveryAST = RecoveryAST;
- Opts.PreserveRecoveryASTType = RecoveryASTType;
Opts.FoldingRanges = FoldingRanges;
Opts.MemoryCleanup = getMemoryCleanupFunction();
@@ -873,7 +861,6 @@
Opts.ClangTidyProvider = ClangTidyOptProvider;
}
