[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 24 05:41:50 PDT 2019
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
More information about the cfe-commits
mailing list