[PATCH] D98824: [Tooling] Handle compilation databases containing commands with double dashes

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D98824#2634715 <https://reviews.llvm.org/D98824#2634715>, @jnykiel wrote:

> I have addressed some of your comments. The second revision is good enough to get the tests to pass - the `injectResourceDir` change was necessary for the one clang-tidy test that started failing after leaving the `--` in. I don't feel familiar enough with clangd specifically to attempt comprehensive fixes there. The clangd issue report specifically mentions `-fsyntax-only` and `-resource-dir` as well - so while not comprehensive, the fix may be good for that too.

There are a lot more argument adjusters that look at the compile flags and they don't stop at `--` though :/ I don't think it is feasible to special case `--` in all of those loops and doesn't happen enough in practice (i.e. we see a filename that starts with `-resource-dir` or `-fsyntax-only` etc.) to justify a more complex solution. So as mentioned in the comments let's ignore these for now.



================
Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:48
+
+      if (Arg == "--")
+        break;
----------------
we should perform this either in all or none. thinking about this again, there are a lot more things that can go wrong but they happen pretty rarely so let's just drop this change and replace the push_back below with `getInsertArgumentAdjuster("-fsyntax-only")(AdjustedArgs, "");`

copying over args once more isn't crucial here as this happen only once before parsing a translation unit, which by far dominates the latency.


================
Comment at: clang/lib/Tooling/InterpolatingCompilationDatabase.cpp:248
+    if (InsertDoubleDash)
+      Result.CommandLine.push_back("--");
     Result.CommandLine.push_back(std::string(Filename));
----------------
the condition should be a function of the `Filename`, not the compile flag we are using. as the original command might not contain the `--` but the filename might still be starting with a `-` or `/` here, right?

So i'd suggest `Filename.startswith("-") || (ClangCLMode && Filename.startswith("/"))`


================
Comment at: clang/lib/Tooling/Tooling.cpp:439
+  for (StringRef Arg : Args) {
+    if (Arg == "--")
+      break;
----------------
similar to comment above, let's just ignore this.


================
Comment at: clang/lib/Tooling/Tooling.cpp:448
+  resourceDir.append(CompilerInvocation::GetResourcesPath(Argv0, MainAddr));
+  Args = getInsertArgumentAdjuster(CommandLineArguments(1, resourceDir),
+                                   ArgumentInsertPosition::END)(Args, "");
----------------
nit:
```
Args = getInsertArgumentAdjuster(("-resource-dir=" + CompilerInvocation::GetResourcesPath(Argv0, MainAddr)).c_str())(Args, "");
```

There's an overload of `getInsertArgumentAdjuster` that takes in a `const char*`


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

https://reviews.llvm.org/D98824