sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:345
CompileCommandsDir);
+ if (!ClangdServerOpts.QueryDriverGlobs.empty())
+ BaseCDB = getSystemIncludeExtractorDatabase(
----------------
if statement is not needed I think, as you return base if empty
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:1
+//===--- SystemIncludeExtractor.cpp ------------------------------*-
C++-*-===//
+//
----------------
This name is a little limiting (e.g. if we want to pull more info from the
driver).
Not a big problem, but especially if you anticipate pulling out more
properties, query-driver or similar might be clearer.
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:66
+ if (StartIt == Lines.end()) {
+ elog("System include extraction: start marker not found.");
+ return {};
----------------
also log output here?
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:91
+ if (!QueryDriverRegex.match(Driver)) {
+ elog("System include extraction: not whitelisted driver {0}", Driver);
+ return {};
----------------
This isn't an error - probably vlog, *maybe* log
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:143
+ }
+ log("System include extractor: succesfully executed {0}", Driver);
+
----------------
also include extracted includes - the info for this weird setup is important to
preserve in the logs I think
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:152
+ for (llvm::StringRef Include : SystemIncludes) {
+ Cmd.CommandLine.push_back("-isystem");
+ Cmd.CommandLine.push_back(Include.str());
----------------
Technically this doesn't work if the command is `clang --driver-mode=cl ...` -
you'll be able to query the info from clang using the default gcc-compatible
syntax, but the actual command-line won't support the -isystem flag.
Not worth fixing I think but a comment?
================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:254
+ assert(Base && "Null base to SystemIncludeExtractor");
+ return llvm::make_unique<SystemIncludeExtractorDatabase>(QueryDriverGlobs,
+ std::move(Base));
----------------
docs says it returns base if globs are empty?
================
Comment at: clang-tools-extra/clangd/test/system-include-extractor.test:1
+# RUN: rm -rf %t.dir && mkdir -p %t.dir
+
----------------
Wow, I'm impressed you managed to test this :-)
Test needs quite a lot of comments to explain what it's doing though, I think.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62804/new/
https://reviews.llvm.org/D62804
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
