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

[Hamza Sood via Phabricator via cfe-commits]

hamzasood marked an inline comment as done.
hamzasood added inline comments.


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:136
+  // Otherwise just check the clang file name.
+  return llvm::sys::path::stem(CmdLine.front()).endswith_lower("clang-cl");
+}
----------------
rnk wrote:
> We support being called "CL.exe", but with our new VS integration, I don't think it matters to check for this case anymore. We may remove it over time.
Should I add the check anyway?


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:140
-    unsigned MissingI, MissingC;
-    auto ArgList = OptTable->ParseArgs(Argv, MissingI, MissingC);
-    for (const auto *Arg : ArgList) {
----------------
rnk wrote:
> Can you not keep using ParseArgs? It takes FlagsToInclude and FlagsToExclude, which you can compute outside the loop now.
Good point, I forgot to move the flags out of the loop when moving the —driver-mode check.

But I don’t think ParseArgs is suitable (see the other comment response).


================
Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156-169
+    // Transform the command line to an llvm ArgList.
+    // Make sure that OldArgs lives for at least as long as this variable as the
+    // arg list contains pointers to the OldArgs strings.
+    llvm::opt::InputArgList ArgList;
+    {
+      // Unfortunately InputArgList requires an array of c-strings whereas we
+      // have an array of std::string; we'll need an intermediate vector.
----------------
rnk wrote:
> I think the old for loop was nicer. I don't think this code needs to change, you should be able to use ParseArgs with the extra flag args.
The problem with the old loop is that we lose aliasing information (e.g. `/W3` becomes `-Wall`). While `ParseOneArg` also performs alias conversion, it gives us indices for each individual argument. The last line of the new loop copies arguments by using that index information to read from `OldArgs`, which gives us the original spelling.


https://reviews.llvm.org/D51321