[PATCH] D62798: Explicitly detect recursive response files

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1110
+  // the need to check for an empty list.
+  FileStack.push_back({"", Argv.size()});
 
----------------
`Argv.size()` can increase, so we could end up potentially popping this off, right? Perhaps something like `std::numeric_limits<size_t>::max` (or just `-1` if you don't feel like being wordy) would be better?


================
Comment at: llvm/lib/Support/CommandLine.cpp:1136
+    // Check for recursize response files.
+    if (std::any_of(FileStack.begin()+1, FileStack.end(), IsEquivalent)) {
+      // This file is recursive, so we leave it in the argument stream and
----------------
Needs to be `clang-format`ted. You can use `git clang-format` to format your entire commit. See https://llvm.org/docs/GettingStarted.html#sending-patches


================
Comment at: llvm/lib/Support/CommandLine.cpp:1157
+    for (ResponseFileRecord &Record : FileStack) {
+      // All records are now bigger. Don't include the @resp file itself, so -1
+      Record.End += ExpandedArgv.size() - 1;
----------------
@rsp file?

I'd say something like "Increase the end of all active records by the number of newly expanded arguments, minus the response file itself"


================
Comment at: llvm/unittests/Support/CommandLineTest.cpp:857
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
   SmallString<128> TestDir;
----------------
Could you expand this test? In particular, we should ensure that we don't have any recursive expansion at all, and we should also test mutual recursion.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62798