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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 6 inline comments as done.
kadircet added inline comments.


================
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.
----------------
sammccall wrote:
> 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.
I agree that it is gonna be somewhat noisy. But I think it is gonna effect 2 different workflows:
- User hits a file with compile flags containing multiple inputs, notices clangd doesn't work, tries to resolve the problem and looks at the logs. since they are not working on the file it is not gonna be noisy in this case and really helpful.
- They are just going to ignore clangd being broken and move on, In this case we'll really turn the logs into garbage but I don't think user will care.

So maybe we can make it vlog instead, and ensure it only shows up to users that care?


================
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.
----------------
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > 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.
> > I agree that it is gonna be somewhat noisy. But I think it is gonna effect 2 different workflows:
> > - User hits a file with compile flags containing multiple inputs, notices clangd doesn't work, tries to resolve the problem and looks at the logs. since they are not working on the file it is not gonna be noisy in this case and really helpful.
> > - They are just going to ignore clangd being broken and move on, In this case we'll really turn the logs into garbage but I don't think user will care.
> > 
> > So maybe we can make it vlog instead, and ensure it only shows up to users that care?
> Another option would be to delete all the OPT_INPUTs and append the actual filename (CompileCommand::Filename)
> 
> This seems like it would define this class of errors out of existence, maybe?
> 
> Layering doesn't make it easy to do that right here, though :-\
yes this sounds great indeed, send out as a separate follow-up in D106639.


================
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.
----------------
sammccall wrote:
> 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...)
send out D106562 to get rid of these.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:232
+  if (ResourceDir &&
+      !ArgList.hasArgNoClaim(driver::options::OPT_resource_dir,
+                             driver::options::OPT_resource_dir_EQ))
----------------
sammccall wrote:
> 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.
keeping the `Has` behaviour just to minimize the impact, happy to drop the check as a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106527