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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
