hamzasood added a comment.
Thanks for the help with this.
I reverted most of my changes (including the parameterised tests) before
committing; I think I implemented all of your suggestions.
Repository:
rL LLVM
https://reviews.llvm.org/D51321
___
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341760: [Tooling] Improve handling of CL-style options
(authored by hamzasood, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51321?vs=163098
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks again for fixing this.
Still a few places I feel the code could be simpler, but will let you decide
how to deal with them.
I would greatly appreciate removing the parameterized tes
hamzasood marked 10 inline comments as done.
hamzasood added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// Determine whether the given file path is the clang-cl executable.
+static bool checkIsCLMode(const std::vector &CmdLine,
+
hamzasood updated this revision to Diff 163098.
hamzasood added a comment.
I've addressed most of your comments and resolved the merge conflicts
introduced in r340838.
https://reviews.llvm.org/D51321
Files:
lib/Tooling/InterpolatingCompilationDatabase.cpp
unittests/Tooling/CompilationDatab
hamzasood added a comment.
Thanks for the detailed feedback; I’ll try to get a new patch up in a few hours.
However I disagree with some of them (see replies).
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// Determine whether the given file path is the cla
sammccall added a comment.
Thanks for fixing this!
Mostly just picky style comments.
In particular, I know that some of the other maintainers here are just as
ignorant of the cl driver as I am, and I want to make sure that it's still
possible to follow the logic and debug unrelated problems with
hamzasood updated this revision to Diff 162872.
hamzasood added a comment.
- Re-uploaded with full context.
Yeah, I encountered these issues while using clangd on Windows. I haven't run
into any clangd issues after applying this patch, but admittedly my usage is
very basic (pretty much just cod
sammccall added a comment.
Hi, sorry about overlooking cl mode flags when adding this, I was blissfully
unaware that `compile_commands.json` could use that syntax :-)
Out of curiosity, are you using this with clangd or some other tool? I'm sure
there are places where clangd injects unixy flags.
hamzasood marked an inline comment as done.
hamzasood added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:136
+ // Otherwise just check the clang file name.
+ return llvm::sys::path::stem(CmdLine.front()).endswith_lower("clang-cl");
+}
--
rnk added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:136
+ // Otherwise just check the clang file name.
+ return llvm::sys::path::stem(CmdLine.front()).endswith_lower("clang-cl");
+}
We support being called "CL.exe", but with
hamzasood updated this revision to Diff 162762.
hamzasood added a comment.
Thanks, I've changed it so that the driver mode is calculated first.
I've also extended the unit test to include the various `--driver-mode`
combinations.
https://reviews.llvm.org/D51321
Files:
lib/Tooling/Interpolati
rnk added inline comments.
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:182-184
+ if (Arg->getOption().matches(driver::options::OPT_driver_mode)) {
+// Process --driver-mode.
+IsCLMode = std::strcmp(Arg->getValue(), "cl") == 0;
---
hamzasood created this revision.
hamzasood added reviewers: rnk, hokein, sammccall.
Herald added a subscriber: cfe-commits.
This patch fixes the handling of clang-cl options in
`InterpolatingCompilationDatabase`.
They were previously ignored completely, which led to a lot of bugs:
- Additional o
14 matches
Mail list logo
