Page MenuHomePhabricator

[clangd] Pass the entire tooling::CompileCommand to CommandMangler
ClosedPublic

Authored by nridge on Sep 13 2022, 1:18 AM.

Details

Summary

This gives CommandMangler access to other fields of
tooling::CompileCommand as well, e.g. Directory.

Diff Detail

Event Timeline

nridge created this revision.Sep 13 2022, 1:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 1:18 AM
Herald added a subscriber: arphaman. · View Herald Transcript
nridge requested review of this revision.Sep 13 2022, 1:18 AM

This is a refactoring in preparation for D133757, please see that for motivation

tom-anders added inline comments.
clang-tools-extra/clangd/indexer/IndexerMain.cpp
150–158

Just wondering, why do you need to convert to a shared_ptr here? Couldn't the lambda just take ownership of the unique_ptr?

nridge added inline comments.Sep 13 2022, 1:10 PM
clang-tools-extra/clangd/indexer/IndexerMain.cpp
150–158

ArgumentsAdjuster is a typedef for std::function which needs to be copyable, not move-only

sammccall added inline comments.Sep 15 2022, 11:32 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

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

nridge added inline comments.Sep 15 2022, 4:53 PM
clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

It's more that QueryDriverDatabase uses 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.)

nridge added inline comments.Sep 25 2022, 12:33 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

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.

nridge updated this revision to Diff 462702.Sep 25 2022, 1:21 AM

Updated to use unique_function rather than inheritance

sammccall accepted this revision.Sep 28 2022, 9:43 PM

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.

clang-tools-extra/clangd/CompileCommands.h
45–48

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 :-/)

clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

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.

185

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?

clang-tools-extra/clangd/indexer/IndexerMain.cpp
150–157

nit: make_shared?

This revision is now accepted and ready to land.Sep 28 2022, 9:43 PM
nridge updated this revision to Diff 470083.Oct 24 2022, 1:55 AM
nridge marked 4 inline comments as done.

Address review comments

(apologies, I'm also very tardy in getting back to this...)

clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

my concern is providing two filenames the query driver database, which needs none

QueryDriverDatabase does in fact use "TargetFile", here.

Based on the usage 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.

sammccall accepted this revision.Oct 24 2022, 1:44 PM

Still LG!

clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

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" :-\

nridge retitled this revision from [clangd] Introduce CompileCommandsAdjuster to [clangd] Pass the entire tooling::CompileCommand to CommandMangler.Oct 29 2022, 5:46 PM
nridge edited the summary of this revision. (Show Details)

(I frequently forget that arc diff does not update the commit message of an existing revision...)

nridge added inline comments.Oct 31 2022, 2:06 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.h
166

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.