Strip preceeding -Xclang when stripping -fcolor-diagnostics or -fdiagnostics-color

[DaanDeMeyer]

Summary

Fixes https://github.com/clangd/clangd/issues/279. We were removing the color options but not the preceeding -Xclang which causes errors since the -Xclang would now apply to the next option in the list of options. Now, when removing a color option, we check if there was a preceeding -Xclang and remove it as well.

[kadircet]

Thanks, LGTM. But please upload the diff with full context, see https://llvm.org/docs/Phabricator.html#phabricator-request-review-web and https://llvm.org/docs/Contributing.html#how-to-submit-a-patch.

We've also discussed this offline and think that a more principled solution is necessary in clangd side to switch from "string manipulations" to a more semantic approach.

In the meantime this fix sounds like a good short term solution, maybe even a long term one for clang-tools that will keep using these adjusters. WDYT @sammccall ?

[sammccall]

LGTM too, though the overall solution space here is a bit sad:

• obviously there's lots of other instances of this pattern that should really be fixed as long as the arg-adjuster API is supported
• driver is such a mess that patching the CompilerInvocation afterwards is also really hard in many cases, though not this one (I prototyped this)

clang/lib/Tooling/ArgumentsAdjusters.cpp

46

Maybe add a FIXME: this should apply to most arg adjusters!

[DaanDeMeyer]

Added FIXME.

[DaanDeMeyer]

I do not have commit access. It would be great if someone could merge this for me.

clang/lib/Tooling/ArgumentsAdjusters.cpp

for (size_t i = 0, e = Args.size(); i < e; ++i) {
       if (llvm::any_of(OutputCommands, [&Arg](llvm::StringRef OutputCommand) {
             return Arg.startswith(OutputCommand);
           }))
         continue;
 
       if (!Arg.startswith("-fcolor-diagnostics") &&
           !Arg.startswith("-fdiagnostics-color"))
         AdjustedArgs.push_back(Args[i]);
+      // If we strip a color option, make sure we strip any preceeding `-Xclang`
+      // option as well.
+      // FIXME: This should be added to most argument adjusters!
+      else if (!AdjustedArgs.empty() && AdjustedArgs.back() == "-Xclang")
+        AdjustedArgs.pop_back();
+
       if (Arg == "-fsyntax-only")
         HasSyntaxOnly = true;
     }
     if (!HasSyntaxOnly)
       AdjustedArgs.push_back("-fsyntax-only");
     return AdjustedArgs;
   };
 }