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

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Thanks for the detailed feedback; I’ll try to get a new patch up in a few hours.

However I disagree with some of them (see replies).



================
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) {
----------------
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.


================
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
----------------
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.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:178
+      {
+        unsigned Included, Excluded;
+        if (IsCLMode) {
----------------
sammccall wrote:
> This seems a bit verbose.
> 
> First, do things actually break if we just use the default 0/0 masks? We're not trying to interpret all the flags, just a few and pass the rest through.
> 
> Failing that, it seems clearer to just use a ternary to initialize Included/Excluded, or inline them completely.
> (Please also drop the extra scope here).
Theoretically it could break without the flags. We need to recognise input files and strip them from the command line. If someone on a UNIX platform has an input file called `/W3`, that’d instead be interpreted as a warning flag and it’ll be left in. Likewise for any file in the root directory with the same spelling as a CL-only argument.

But yeah, I’ll inline the flags with a ternary.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:205
+      if (const auto GivenStd = isStdArg(*Arg)) {
+        if (*GivenStd != LangStandard::lang_unspecified)
+          Std = *GivenStd;
----------------
sammccall wrote:
> prefer *either* optional<> or allowing the function to return lang_unspecified, but not both. (There's no way a user can *explicitly* pass a flag saying the language is unspecified, right?)
Kind of: `-std=hello_there` is parsed as an unspecified language. We still want to strip the flag in this case, which won’t be possible if we also use the unspecified value to denote a lack of an `-std` flag.


================
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);
----------------
sammccall wrote:
> can we unify as `addTypeFlag(TargetType, Mode, Result.CommandLine)`?
To clarify, you mean extract this into a helper function?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:258
+
+    if (IsCLMode)
+      return Opt.matches(driver::options::OPT__SLASH_Fa) ||
----------------
sammccall wrote:
> Why do we need to take IsClMode into account while reading flags?
> How would `OPT__SLASH_Fa` get parsed if we weren't in CL mode? Would we care?
> 
> (and below)
It was an optimisation attempt in that we can do 5 less comparisons in the case where we know that there aren’t going to be any CL flags. Maybe I was trying too hard to micro-optimise...


================
Comment at: unittests/Tooling/CompilationDatabaseTest.cpp:652
+
+class InterpolateTest
+  : public ::testing::TestWithParam<std::tuple<DriverExe, DriverMode>> {
----------------
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.


https://reviews.llvm.org/D51321