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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
