hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+ ConfigCompile.cpp
ConfigYAML.cpp
Diagnostics.cpp
----------------
we have a few config-related files now, I wonder would it make sense to create
a config/ subdir?
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:47
+
+ std::string RegexError = "";
+ llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text) {
----------------
this is only used in method `compileRegex`, looks like can be moved to a local
variable? is there life-time issue?
================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:96
+
+ constexpr static auto Error = llvm::SourceMgr::DK_Error;
+ void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
----------------
this member seems unused.
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:32
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+ /// Absolute path to file we're targeting. Unix slashes.
----------------
nit: I find the name `Params` is too general, it implies less information. When
I read the code using `Params`, I have to go-to-definition to see what it
means.
if we only have a single member in this struct, maybe just use it directly.
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:40
+///
+/// Fragments are compiled by Providers when first loaded, and cached for
reuse.
+/// Like a compiled program, this is good for performance and also encourages
----------------
I might miss the context, where is the provider, not added yet?
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:47
+ /// This always produces a usable compiled fragment (errors are recovered).
+ explicit CompiledFragment(Fragment, DiagnosticCallback);
+
----------------
Would it be more nature to have a function `compile`(or so) to do the actual
compile (Fragment -> CompiledFragment) rather than doing it in a constructor?
================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:40
+TEST_F(ConfigCompileTests, Condition) {
+ // No condition.
+ Frag.CompileFlags.Add.emplace_back("X");
----------------
my first impression was that each `// XXX` is a separated test, but it turns
out not, the tests here seem to be stateful -- `Frag` keeps being modified. I
think it is intentional, but it is hard for readers to track given that the
code is long.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
