[PATCH] D133757: [clangd] Perform system include extraction inside CommandMangler

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I believe this addresses the remaining review comments. I will follow up with a patch to rename QueryDriverDatabase.{h,cpp} to SystemIncludeExtractor,{h,cpp}.



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:318
 /// compilation database.
-class QueryDriverDatabase : public DelegatingCDB {
+class SystemIncludeExtractor : public CompileCommandsAdjuster {
 public:
----------------
sammccall wrote:
> 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
> 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

I went with the first one.

A couple of minor deviations from what you suggested in [this comment](https://reviews.llvm.org/D133756#inline-1300154) are:

 1. I declared a typedef for the `unique_function` at namespace scope rather than inside `CommandMangler`, so that I can use it in the declaration of `getSystemIncludeExtractor()`
 2. I named the typedef `SystemIncludeExtractorFn` to avoid a conflict with the name of the actual implementing class

Let me know if you prefer some other choices here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133757