[PATCH] D62804: [clangd] Enable extraction of system includes from custom toolchains

[Sam McCall via Phabricator via cfe-commits]

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