[PATCH] D62798: Explicitly detect recursive response files

Shoaib Meenai via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 5 12:05:32 PDT 2019


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()});
 
----------------
chrisglover wrote:
> smeenai wrote:
> > `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?
> If I got this correct, this particular node will never be popped off. There's an assert at the end of the function that checks that the value of this node did indeed expand out to the size of the argument list
> assert(Argv.size() == FileStack.back().End);
> But, when I think about it, this assert isn't checking any particular post-condition, it's just validating that the algorithm is correct. For this kind of code, which is a bit complex, I feel like this is reasonable choice to defend against future changes, but really it comes down to a philosophical choice that I'm not strongly attached to one way of the other. 
Yeah, I understand now. I forgot to account for the dummy entry also having its End incremented in the loop below.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1167
+
+  assert(Argv.size() == FileStack.back().End);
   return AllExpanded;
----------------
jyknight wrote:
> Worth putting a comment here as to why this is the case.
Yeah, this would address the confusion I had :)


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

https://reviews.llvm.org/D62798





More information about the llvm-commits mailing list