[PATCH] D133756: [clangd] Introduce CompileCommandsAdjuster

Mon Oct 24 13:44:28 PDT 2022

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