sammccall accepted this revision.
sammccall added a comment.
Still LG!
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:165
+// process a file (possibly different from the one in the command).
+class CompileCommandsAdjuster {
+public:
----------------
nridge wrote:
> 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.
> QueryDriverDatabase does in fact use "TargetFile", here.
Oops, of course. Well, my preference would be to pass a CompileCommand and just
have it reference the filename in there, but happy with what you decide.
> 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...)
Hmm, I would actually think it must be safe to overwrite Filename with File
before calling the mangler. Because we do this already if we
`transferCompileCommand`...
So I'm torn between "let's not work hard to preserve arbitrary implementation
details" and "let's not make another subtle change here if we can avoid it" :-\
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
