[PATCH] D92012: [clangd][query-driver] Extract target

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this seems really useful!



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:59
 
-std::vector<std::string> parseDriverOutput(llvm::StringRef Output) {
+std::pair<std::vector<std::string>, std::string>
+parseDriverOutput(llvm::StringRef Output) {
----------------
define a little struct holding these two, for clarity?


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:78
+  } else {
+    // Start from the beginning if target was not found.
+    StartIt = Lines.begin();
----------------
This part seems pretty confusing :-)
You could just search for Target: independently of the current scanning - I'm not terribly worried about it appearing in the middle of the include list!

If we want to be strict though, it might be time to rewrite to be more stateful e.g. a loop consuming chunks from an ArrayRef<StringRef>, or a state machine, or something.



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:87
+    return {SystemIncludes, Target};
   }
   ++StartIt;
----------------
this changes the error behavior: intentional?


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:212
+  if (!Target.empty()) {
+    Cmd.CommandLine.push_back("-target");
+    Cmd.CommandLine.push_back(Target);
----------------
`clang -target foo test.cc` seems to be a hard error in the driver if the target is unknown.
(vs likely *some* functionality if we just didn't set the driver)

so this could regress some scenarios. Can we mitigate that?
(It's possible that we're running the driver in a mode where we proceed anyway, but I can't remember :-()


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:307
+    addSystemIncludes(*Cmd, SystemIncludesAndTarget.first);
+    return setTarget(*Cmd, SystemIncludesAndTarget.second);
   }
----------------
nit: while here, add std::move?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92012