[PATCH] D99523: [clangd] Use command line adjusters for inserting compile flags

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: cfe-commits.

Thanks for fixing!



================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:225
 
+  Cmd = tooling::getInsertArgumentAdjuster(
+      ToAppend, tooling::ArgumentInsertPosition::END)(Cmd, "");
----------------
This extra copy is really silly :-(
please `move(ToAppend)` and make the whole thing conditional on `!ToAppend.empty()` (even though that'll be pretty much always...). Sadly move(Cmd) won't work.

One day if we feel like shaving a yak, we could add migrate to an ArgsAdjuster interface that takes a mutable CompileCommand&...


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:271
       Out.Apply.push_back([Add(std::move(Add))](const Params &, Config &C) {
         C.CompileFlags.Edits.push_back([Add](std::vector<std::string> &Args) {
+          auto It = llvm::find(Args, "--");
----------------
Hmm, with this new logic in place, if we "normalized" command lines by moving the filename to the end, we'd resolve https://github.com/clangd/clangd/issues/555 without having to complicate the config file model.

I wonder how hard that is...


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:272
         C.CompileFlags.Edits.push_back([Add](std::vector<std::string> &Args) {
-          Args.insert(Args.end(), Add.begin(), Add.end());
+          auto It = llvm::find(Args, "--");
+          Args.insert(It, Add.begin(), Add.end());
----------------
maybe a comment "The point to insert at - usually the end"?
(Since this looks suspiciously like an unhandled error case)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99523/new/

https://reviews.llvm.org/D99523