nridge added inline comments.
================
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:
> > 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"
> :-\
I left things as is for now. I can explore simplifying the signature of
SystemIncludeExtractor to be just a tooling::CompileCommand in a follow-up
patch.
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
