[PATCH] D133756: [clangd] Introduce CompileCommandsAdjuster

Wed Sep 28 21:43:13 PDT 2022

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry about the long turnaround; have had some less-fun-than-code things to deal with :-) Thanks as always for patience...

Main thing I'd still like is to split into two separate `function` types defined where they're passed, and to drop the `File` param from the one that doesn't need it.

But stamping it now, land a version of it you're happy with and we can tweak later if need be.

================
Comment at: clang-tools-extra/clangd/CompileCommands.h:45

-  void adjust(std::vector<std::string> &Cmd, llvm::StringRef File) const;
-  explicit operator clang::tooling::ArgumentsAdjuster() &&;
+  void operator()(tooling::CompileCommand &Cmd, llvm::StringRef File) const;

----------------
while here, can we rename File => TargetFile or something?
Maybe with a comment like "Cmd may describe compilation of a different file, and will be updated for parsing TargetFile"?

(I was confused while reviewing about what Cmd.Filename vs File were used for, and I think I wrote this code :-/)

================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:184
              std::vector<std::string> FallbackFlags = {},
-             tooling::ArgumentsAdjuster Adjuster = nullptr);
+             CompileCommandsAdjuster Adjuster = nullptr);

----------------
nit: if we're not actually using `tooling::ArgumentsAdjuster`, I think it'd be clearer not to reuse the name "Adjuster".

Calling this OverlayCDB::CommandMangler/Mangler seems neat to me (it lets a human reader know what's up) but otherwise a synonym like `CommandEdits` might work?

================
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:
> 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.

================
Comment at: clang-tools-extra/clangd/indexer/IndexerMain.cpp:150
       clang::tooling::ArgumentsAdjuster(
-          clang::clangd::CommandMangler::detect()));
+          [Mangler = std::shared_ptr<clang::clangd::CommandMangler>(
+               new clang::clangd::CommandMangler(
----------------
nit: make_shared?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133756/new/

https://reviews.llvm.org/D133756