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<std::string> &CmdLine,
+ const llvm::opt::OptTable &OptTable) {
----------------
hamzasood wrote:
> sammccall wrote:
> > nit: a two-value enum would be clearer and allow for terser variable names
> > (`Mode`)
> The advantage of a Boolean is that it makes the checks simpler. E.g. `if
> (isCL)` instead of `if (mode == DriverMode::CL)` or something.
>
> Happy to change it though if you still disagree.
Also, there're more than just 2 driver modes. But we only care about cl and not
cl.
================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156
+
+ // Transform the command line to an llvm ArgList.
+ // Make sure that OldArgs lives for at least as long as this variable as
the
----------------
hamzasood wrote:
> sammccall wrote:
> > would you mind reverting this change and just wrapping the old Argv in an
> > InputArgList?
> > I'm not sure the lifetime comments and std::transform aid readability here.
> The change was more about safety than readability. The old approach left an
> InputArgList of dangling pointers after moving the new args into the cmd
> object. This way there’s no way to accidentally access deallocated memory by
> using the InputArgList.
>
> As above, happy to change if you still disagree.
I've restructured it so hopefully this is less of a concern.
================
Comment at: unittests/Tooling/CompilationDatabaseTest.cpp:652
+
+class InterpolateTest
+ : public ::testing::TestWithParam<std::tuple<DriverExe, DriverMode>> {
----------------
hamzasood wrote:
> sammccall wrote:
> > I'm not sure the parameterized test is the right model here.
> > The `INSTANTIATE_TEST_P` and friends makes the test fixture pretty
> > complicated, and failure messages hard to relate to input conditions.
> > Then the actual tests have a bunch of assertions conditional on which mode
> > we're in.
> > Finally, we don't actually test any mixed-mode database.
> >
> > Could we write this a little more directly?:
> > - pass the driver mode flag to `add()` in the normal way, in the Flags
> > param
> > - require callers to "add" to pass "clang" or "clang-cl". (It's OK that
> > this makes existing cases a bit more verbose)
> > - explicitly test the clang-cl and --driver-mode cases we care about,
> > rather than running every assertion in every configuration
> >
> > e.g.
> > ```
> > TEST_F(InterpolateTest, ClangCL) {
> > add("foo.cc", "clang");
> > add("bar.cc", "clang-cl");
> > add("baz.cc", "clang --driver-mode=clang-cl");
> >
> > EXPECT_EQ(getCommand("a/bar.h"), "clang-cl -D a/baz.cc /TP");
> > }
> > ```
> The intent was to make sure that the existing cases (that shouldn’t depend on
> the driver mode) don’t break, which admittedly happened while I was working
> on the patch.
>
> Most of the tests aren’t conditional on the mode, and I think it’s important
> that they’re tested in all configurations. The parameterisation also lets us
> test as `clang-cl`, `clang-cl —driver-mode=cl`, and `clang —driver-mode=cl`.
> Which are all valid ways of using CL mode.
To clarify: the parameterisation runs the test 6 times. The `isTestingCL()`
check narrows that down to 3.
https://reviews.llvm.org/D51321
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
