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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
