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

[Sam McCall via Phabricator via cfe-commits]

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 tests, as that's the place where I feel least confident I can understand exactly what the intended behavior is.



================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:141
+    {
+      SmallVector<const char *, 8> TmpArgv;
+      TmpArgv.reserve(OldArgs.size());
----------------
please remove premature optimizations (SmallVector, reserve) - this function is not (any more) performance-critical


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:143
+      TmpArgv.reserve(OldArgs.size());
+      llvm::transform(OldArgs, std::back_inserter(TmpArgv),
+                      [](const std::string &S) { return S.c_str(); });
----------------
simple loop is more readable than transform()


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:192
+    const auto DriverModeName =
+      OptTable.getOption(driver::options::OPT_driver_mode).getPrefixedName();
+
----------------
can we just inline this ("--driver-mode") like we do with other specific we need in their string form (std etc)?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:196
+    for (StringRef S : llvm::reverse(CmdLine)) {
+      if (S.startswith(DriverModeName))
+        return S.drop_front(DriverModeName.length()) == "cl";
----------------
```
if (S.consume_front(...))
  return S == "cl";
```


================
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:
> 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.
I do find `if (mode == DriverMode::CL)` much clearer.

"CL" isn't a particularly clear name for people who don't deal with windows much, and "!isCL" isn't a great name for the GCC-compatible driver.


================
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:
> 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.
I think a comment `// ArgList is no longer valid` would suffice, or storing the ArgList in an `Optional` and resetting it.

In principle the restructuring seems fine, but it introduced new issues: the boundaries and data flow between the constructor and `processInputCommandLine` is unclear. (It reads from parameters which are derived from `Cmd.CommandLine` and overwrites `Cmd.CommandLine` itself, along with other state. Its name doesn't help clarify what its responsibility is.

If you want to keep this function separate, I'd suggest:
 - make it static to avoid confusion about state while the constructor is running
 - call it `parseCommandLine` to reflect the processing it's doing
 - return `tuple<vector<String>, DriverMode, LangStandard, Optional<Type>>`
 - call it as `std::tie(Cmd.CommandLine, Mode, ...) = parseCommandLine(...)`
But it also seems fine as part of the constructor.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:231
                        : Type;
-      Result.CommandLine.push_back("-x");
-      Result.CommandLine.push_back(types::getTypeName(TargetType));
+      if (IsCLMode) {
+        const StringRef Flag = toCLFlag(TargetType);
----------------
hamzasood wrote:
> sammccall wrote:
> > can we unify as `addTypeFlag(TargetType, Mode, Result.CommandLine)`?
> To clarify, you mean extract this into a helper function?
Right - you already have the helper function `toCLflag`, I'd suggest extending/renaming it so it covers both CL and GCC and fits the call site more conveniently.


================
Comment at: unittests/Tooling/CompilationDatabaseTest.cpp:652
+
+class InterpolateTest
+  : public ::testing::TestWithParam<std::tuple<DriverExe, DriverMode>> {
----------------
hamzasood wrote:
> 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.
Understood. I don't think the extra test coverage here is worth the infrastructure complexity, and would strongly prefer to add a handful of tests covering driver mode interactions instead.


https://reviews.llvm.org/D51321