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