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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits