ilya-biryukov added a comment.
Thanks, this looks very good!
Leaving a few nitpicky comments, but nothing important really.
Two more general NITs:
- could we update the title and description of this revision to mirror that it
focuses on code in tooling rather than in clangd?
- maybe land the clangd parts into a separate change for nicer VCS history?
================
Comment at: clang/include/clang/Tooling/CompilationDatabase.h:220
+std::unique_ptr<CompilationDatabase>
+getTargetAndModeAdderDatabase(std::unique_ptr<CompilationDatabase> Base);
+
----------------
NIT: in the spirit of a previous name, we could shorten the name
`inferTargetAndDriverMode(…)`
================
Comment at: clang/lib/Tooling/CMakeLists.txt:25
StandaloneExecution.cpp
+ TargetAndModeAdderDatabase.cpp
Tooling.cpp
----------------
NIT: `GuessTargetAndDriverModeCompilationDatabase` would align better with the
names of other files that define compilation databases.
It looks a little long, though, maybe there's a better word to capture
`TargetAndMode`
================
Comment at: clang/lib/Tooling/TargetAndModeAdderDatabase.cpp:39
+ std::vector<CompileCommand>
+ addTargetAndMode(std::vector<CompileCommand> &&Cmds) const {
+ for (auto &Cmd : Cmds) {
----------------
NIT: maybe accept by value to simplify the code?
A cost of an extra move constructor here is negligible.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63194/new/
https://reviews.llvm.org/D63194
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
