[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Kadir and I discussed this a bunch today. FWIW I agree with this patch: sticking the query-driver step in the middle of the pipeline is the right way to solve these bugs.
Putting it on the end and hoping to get away with it feels risky to me.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:477
         std::make_unique<DirectoryBasedGlobalCompilationDatabase>(CDBOpts);
-    BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
-                                     std::move(BaseCDB));
+    Extractor =
+        getSystemIncludeExtractor(llvm::makeArrayRef(Opts.QueryDriverGlobs));
----------------
this is carefully preserving existing behavior: --query-driver has no effect when --compile_commands_from=lsp

I don't think that behavior is intended, we can remove it either here or in a followup patch


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:79
 
+  /// Methods exposed for testing.
+  llvm::Optional<tooling::CompileCommand> getCompileCommand(PathRef File);
----------------
(if the unit tests gets converted back to a lit test, we should probably remove this)


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:310
+  //    that needs to be invoked may come from the CompileFlags->Compiler key
+  //  - BEFORE resolveDriver() because that can mess up the driver path,
+  //    e.g. changing gcc to /path/to/clang/bin/gcc
----------------
kadircet wrote:
> why is this necessary? i.e. are there cases where heuristics in clang-driver today cannot identify the same set of includes we would extract from a system installed `gcc` ?
> 
> because if we were to get rid of this requirement, we can just run this as an extra arg adjuster after running command mangler, without changing the logic in CommandMangler at all (and also rendering the underlying patch unnecessary (i am still not sure if it's needed, but need to take a closer look there)).
I think Kadir is suggesting allowing a bug (handling of `gcc` + `--query-driver`) that we think is unimportant, in order to simplify the implementation.

After staring at this for a while, I think having the CommandMangler encapsulate all the ugly stuff we do here is the right *interface*, and that justifies the implementation being a bit ugly.


================
Comment at: clang-tools-extra/clangd/CompileCommands.h:56
 private:
+  std::unique_ptr<CompileCommandsAdjuster> SystemIncludeExtractor;
   Memoize<llvm::StringMap<std::string>> ResolvedDrivers;
----------------
caches notwithstanding, CommandMangler is a "simple struct" - I think this field should be public for consistency.
(And set manually rather than passed to detect() - there's no detection involved)


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:318
 /// compilation database.
-class QueryDriverDatabase : public DelegatingCDB {
+class SystemIncludeExtractor : public CompileCommandsAdjuster {
 public:
----------------
this is renaming the class, but not the file, the flag, etc.
The actual name aside, such a rename should probably be more complete and a separate commit. If we keep the `--query-driver` flag we'll have at least two names, so we have to weigh that against the betterness of the name too.

(I agree that the current name is somewhat confusing. The new name is somewhat inaccurate: we're extracting from a compiler rather than the (operating?) system, and we extract more than includes. I prefer the new one on balance but maybe `ToolchainFlagsExtractor` or something would be even better...)


================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:74
   FeatureModuleSet FeatureModules;
+  llvm::Optional<ClangdLSPServer> Server;
 
----------------
The new destruction order looks much less obvious, even if we can get away with it at the moment.
Maybe add an accessor instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133757/new/

https://reviews.llvm.org/D133757