[PATCH] D95099: [clang-scan-deps] : Support -- in clang command lines.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Thanks for splitting this out! The looks good, just a few nits in line below. I also have a question about the test.



================
Comment at: clang/lib/Tooling/Tooling.cpp:440
+  // names.
+  auto ArgsEnd = Args.end();
+  for (auto I = Args.begin(), E = Args.end(); I != E; ++I) {
----------------
I think another name that doesn't sound like a synonym of `Args.end()` would be more clear. Maybe `LastFlag` or `FlagsEnd`?


================
Comment at: clang/test/ClangScanDeps/Inputs/regular_cdb.json:12-16
 {
   "directory": "DIR",
-  "command": "clang -E DIR/regular_cdb_input.cpp -IInputs -o adena.o",
+  "command": "clang -E -IInputs -o adena.o -- DIR/regular_cdb_input.cpp",
   "file": "DIR/regular_cdb_input.cpp"
 }
----------------
It's not obvious to me how this exercises the new `-resource-dir` logic. Can you walk me through it?


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:424
         bool HasResourceDir = false;
+        auto ArgsEnd = llvm::find(Args, "--");
+
----------------
I think it'd be a bit clearer to name this `LastFlag` or `LastOption` or `FlagsEnd` or something, since this is not the last argument, just the last flag/option. Especially helpful on the last usage below when referencing the range from `ArgsEnd` to `Args.end()`.


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:427
         // We need to find the last -o value.
         if (!Args.empty()) {
+          // Reverse scan, starting at the end or at the element before "--".
----------------
Should this condition be instead check for `LastFlag != Args.begin()`?


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:430
+          auto ArgsRBegin = llvm::make_reverse_iterator(ArgsEnd);
+          for (auto It = ArgsRBegin; It != Args.rend(); ++It) {
+            StringRef Arg = *It;
----------------
Nit: since you're touching this line anyway, would be nice to update it to follow the usual LLVM style of calling `rend()` just once instead of on every iteration:
```
for (auto RB = llvm::make_reverse_iterator(LastFlag), RI = RB, RE = Args.rend();
     RI != RE; ++RI) {
```
Up to you, but I find the initialism `RI` a bit more indicative of a reverse iterator... this patch touches every use so you could rename it without pulling in any extra diff. If you prefer the names you have that's fine too though.

Also, if you narrow the scope of `RB` / `ArgsRBegin` you can probably reduce nesting by deleting the redundant `if`, but that'd be nice to commit separately (in a follow up?) to avoid cluttering up this patch with NFC indentation changes.


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:434
+              if (Arg == "-o" && It != ArgsRBegin)
+                LastO = *(It - 1); // Next argument (reverse iterator)
               else if (Arg.startswith("-o"))
----------------
Does `RI[-1]` work here? Personally I find `x[y]` more clear than `*(x + y)`.


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:480
         }
+        AdjustedArgs.insert(AdjustedArgs.end(), ArgsEnd, Args.end());
         return AdjustedArgs;
----------------
Can we use `AdjustedArgs.append(LastFlag, Args.end())` here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95099/new/

https://reviews.llvm.org/D95099