This gives CommandMangler access to other fields of
tooling::CompileCommand as well, e.g. Directory.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
clang-tools-extra/clangd/indexer/IndexerMain.cpp | ||
---|---|---|
150–158 | ArgumentsAdjuster is a typedef for std::function which needs to be copyable, not move-only |
clang-tools-extra/clangd/GlobalCompilationDatabase.h | ||
---|---|---|
166 | I have a couple of concerns with this interface:
The immediate problem being solved is the type of CommandMangler::SystemIncludeExtractor, right? |
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.) |
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. |
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? (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 |
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.
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). (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? |
(apologies, I'm also very tardy in getting back to this...)
clang-tools-extra/clangd/GlobalCompilationDatabase.h | ||
---|---|---|
166 |
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. |
Still LG!
clang-tools-extra/clangd/GlobalCompilationDatabase.h | ||
---|---|---|
166 |
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.
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" :-\ |
(I frequently forget that arc diff does not update the commit message of an existing revision...)
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. |
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 :-/)