[PATCH] D133756: [clangd] Introduce CompileCommandsAdjuster

[Nathan Ridge via Phabricator via cfe-commits]

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