[PATCH] D106527: [clangd] Canonicalize compile flags before applying edits

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:210
+  if (!ArgList.hasArgNoClaim(driver::options::OPT__DASH_DASH)) {
+    // In theory there might be more than one input, but clangd can't deal with
+    // them anyway.
----------------
More than one input is a really annoying error to diagnose, would we want to detect and log it here? Downside is this code runs *really* often.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:221
 
-  // Check whether the flag exists, either as -flag or -flag=*
-  auto Has = [&](llvm::StringRef Flag) {
-    for (llvm::StringRef Arg : Cmd) {
-      if (Arg.consume_front(Flag) && (Arg.empty() || Arg[0] == '='))
-        return true;
-    }
-    return false;
-  };
-
   // clangd should not write files to disk, including dependency files
   // requested on the command line.
----------------
These are copies of the rendered arg list, i think many of them we could replace with calls to ArgList.erase before rendering the arg list.
(e.g. syntax-only... if we erased -save-temps and turned off color diagnostics in Compiler.cpp, I think that covers everything)

... but wait, is "rendering the arglist" even possible?

(I want to expand the scope of this patch unneccesarily, but it might be a way to offset the extra costs...)


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:232
+  if (ResourceDir &&
+      !ArgList.hasArgNoClaim(driver::options::OPT_resource_dir,
+                             driver::options::OPT_resource_dir_EQ))
----------------
Technically the hasArgNoClaim is stale, we've applied edits to the command since then. Probably fine though, not much use for setting resource-dir. (maybe we should even drop this check)

hasArgNoClaim is probably cheaper than the old scan we were doing, but its impl is crazy: not a simple scan, but also not a simple lookup.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:238
   // `--sysroot` is a superset of the `-isysroot` argument.
-  if (Sysroot && !Has("-isysroot") && !Has("--sysroot")) {
+  if (Sysroot && !ArgList.hasArgNoClaim(driver::options::OPT__sysroot,
+                                        driver::options::OPT__sysroot_EQ,
----------------
OTOH the edits may well set sysroot or isysroot, i'm not sure this is valid :-(


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:246
   if (!ToAppend.empty()) {
     Cmd = tooling::getInsertArgumentAdjuster(
         std::move(ToAppend), tooling::ArgumentInsertPosition::END)(Cmd, "");
----------------
while here and worrying about efficiency - should we just search for '--' ourselves?
This is an extra copy (due to the ArgsAdjuster interface)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106527