nridge added a comment.
(apologies, I'm also very tardy in getting back to this...)
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:165
+// process a file (possibly different from the one in the command).
+class CompileCommandsAdjuster {
+public:
----------------
sammccall wrote:
> nridge wrote:
> > nridge wrote:
> > > sammccall wrote:
> > > > I have a couple of concerns with this interface:
> > > > - we seem to be stretching to cover {mangler, querydriver} with one
> > > > interface, but there's no particular reason to - we never use it
> > > > polymorphically.
> > > > - the contract is very vague. If it's just "mutate flags" then some
> > > > sort of generic `function<void(CompileCommand&)>` seems sufficient
> > > > - `File` feels out-of-place - it's purely an input for the mangler. If
> > > > query-driver needs an extra input, will we add that too?
> > > > - either `CompileCommand` or filename+argv seems reasonable, providing
> > > > CompileCommand+argv is confusing.
> > > > - the name is hard to distinguish from tooling::ArgumentsAdjuster
> > > > (which is a bad interface)
> > > >
> > > > The immediate problem being solved is the type of
> > > > CommandMangler::SystemIncludeExtractor, right?
> > > > Can that just be `unique_function<void(vector<string>&, StringRef)>` or
> > > > so? Possibly behind `using SystemIncludeExtractor = ...`.
> > > It's more that `QueryDriverDatabase`
> > > [uses](https://searchfox.org/llvm/rev/f213128b292da85f68eeebbb68cba1541e1c39e2/clang-tools-extra/clangd/QueryDriverDatabase.cpp#354)
> > > the `Directory` field of `tooling::CompileCommand` in addition to the
> > > arguments.
> > >
> > > We could add the directory as another argument to the function, but at
> > > that point why not group the arguments into `tooling::CompileCommand`
> > > which is more semantically meaningful?
> > >
> > > As for polymorphism vs. `unique_function`, I don't feel strongly about
> > > that, happy to change that up. (I do find `function` more annoying to
> > > debug because `step into` at a call site in the debugger steps into some
> > > hard-to-read library code, but that's probably better solved at the
> > > debugger tooling level.)
> > Forgot to mention one other subtlety here: because `QueryDriverDatabase`
> > needs to operate on a `tooling::CompileCommand`, the interface between
> > `CommandMangler` and `OverlayCDB` needs to be widened to accommodate
> > passing a `tooling::CompileCommand`, i.e. it can't be `ArgumentsAdjuster`
> > any more.
> >
> > This is why I reused `CompileCommandsAdjuster` as the type used to pass
> > `CommandMangler` to `OverlayCDB`, and the type used to pass
> > `SystemIncludeExtractor` to `CommandMangler`. It's not used
> > //polymorphically//, we just need an interface that can accept a
> > `CompileCommand` in both places; they could be two different types if we
> > prefer.
> > It's more that QueryDriverDatabase uses the Directory field of
> > tooling::CompileCommand in addition to the arguments.
>
> Yes, that makes sense - my concern is providing **two** filenames the query
> driver database, which needs none. The first one is semantically clear and
> the function could potentially use it. The second one is completely
> meaningless though.
>
> > the interface between CommandMangler and OverlayCDB needs to be widened to
> > accommodate passing a tooling::CompileCommand
>
> Right. IIUC the reason we're using these abstract types (as opposed to
> directly using CommandMangler and some concrete SystemIncludeExtractor) is
> dependency injection: we don't want OverlayCDB to depend on CommandMangler,
> or CommandMangler to depend on SystemIncludeExtractor.
>
> For DI, I think we should specify these types independently where they're
> consumed, i.e.
> ```
> class OverlayCDB {
> using CommandMangler = unique_function<void(CompileCommand&, StringRef
> NewFilename)>;
> OverlayCDB(..., CommandMangler M = nullptr);
> }
> ```
> This avoids spurious dependencies in the *other* direction (e.g.
> CommandMangler should not have to depend on OverlayCDB).
> We don't give readers the impression that mangler & extractor are
> interchangeable somehow.
> And things needed by one (e.g if CommandMangler needs 6 more filename
> params!) needn't affect the other.
>
> (I violated all these ideas in reusing tooling::ArgumentsAdjuster here, it
> was a mistake)
>
> Ack on the debugger/stacktrace problems with type-erased functions though, I
> wish I had a good answer.
> my concern is providing two filenames the query driver database, which needs
> none
QueryDriverDatabase does in fact use "TargetFile",
[here](https://searchfox.org/llvm/rev/016c83047f6ffdca8e83176733e5777176702e79/clang-tools-extra/clangd/QueryDriverDatabase.cpp#340).
Based on the
[usage](https://searchfox.org/llvm/rev/016c83047f6ffdca8e83176733e5777176702e79/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp#757,761)
in `OverlayCDB::getCompileCommands()`, I don't think we can assume that the
`File` passed to `QueryDriverDatabase::getCompileCommand()` is the same as the
`Filename` of the `CompileCommand` that the base CDB returns (otherwise, the
mangler could rely on the same assumption and wouldn't need the `TargetFile`
parameter either).
---
Your point about using different `unique_function` typedefs for CommandMangler
and SystemIncludeExtractor, and declaring them close to where they're consumed,
makes sense though, I changed that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133756/new/
https://reviews.llvm.org/D133756
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
