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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
