[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

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