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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the feedback! Yeah I'd be wary of introducing a corner case where the user passes `--query-driver`, and there is in fact a driver available to query in `PATH`, but we end up not querying it. Even if the outcome is the same in cases we can think of, it feels like a footgun that's going to bite some user somewhere.



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:318
 /// compilation database.
-class QueryDriverDatabase : public DelegatingCDB {
+class SystemIncludeExtractor : public CompileCommandsAdjuster {
 public:
----------------
sammccall wrote:
> 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...)
Mostly I wanted to drop the `Database` from `QueryDriverDatabase` because it was no longer a `GlobalCompilationDatabase`, and just `QueryDriver` didn't sound right (a verb, not a noun).

I do see the value in sticking to the terminology already used in the flag. If you're happy with the `CompileCommandsAdjuster` abstraction (and its name), we could make it `QueryDriverAdjuster`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133757