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