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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I think this patch is great apart from the testing issue.

(I do see the value in a good end-to-end test for the exact bug being fixed, but would also be fine with this being a unit-test of CommandMangler with the query driver part mocked out)



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:318
 /// compilation database.
-class QueryDriverDatabase : public DelegatingCDB {
+class SystemIncludeExtractor : public CompileCommandsAdjuster {
 public:
----------------
nridge wrote:
> 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`?
> Mostly I wanted to drop the Database from QueryDriverDatabase

Yup, makes sense. We should drop it from the filename too, right? On reflection renaming the class in this patch and the file in a subsequent one also seems reasonable.

> If you're happy with the CompileCommandsAdjuster abstraction (and its name), we could make it QueryDriverAdjuster?

As mentioned on the other patch I'm not really happy with the Adjuster name, too much trauma from tooling::ArgumentsAdjuster :-)
Having thought about it more, I like `SystemIncludeExtractor` a lot, even if it doesn't totally cover everything.

So I'd prefer either:
 - best name: rename everything to `SystemIncludeExtractor` except the flag. (i.e. this patch, with a followup commit renaming the file)
 - most consistent: rename everything to `QueryDriver` - it's a verb, but it's also a functor

up to you


================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:242
+CompileFlags:
+  Compiler: gcc
+  )yaml";
----------------
kadircet wrote:
> nridge wrote:
> > To exercise `QueryDriverDatabase`, this test assumes that the machine running the test will have a `gcc` somewhere in its path, and that querying it for includes will result in at least one `-isystem` flag being added to the command line.
> > 
> > Is this a reasonable assumption for buildbots? Or do we need to introduce some kind of abstraction so that the test doesn't actually try to find and execute gcc?
> i think it's better to have this as a lit test rather than a unittest. that way we can set up the temp env in the lit test with appropriate paths and check for the output of `clangd --check=foo.cc`. (we should also have a mock shell script called gcc that'll print out expected lines, we already have this in `clang-tools-extra/clangd/test/system-include-extractor.test`
> 
> because this doesn't only rely on existence of `gcc` on buildbots, but also anyone that wants to run clangd-check locally (and might interact weirdly depending on where/which gcc is installed).
Agree, I know this is a pain but it's not just buildbots: people run these tests when building official llvm releases or distro packages for all sorts of systems, and I think a dep on gcc might be a problem. (e.g. on windows, but also I think a basic FreeBSD install has clang but not gcc these days?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133757