[PATCH] D62798: Explicitly detect recursive response files

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

Comment updates SGTM.



================
Comment at: llvm/lib/Support/CommandLine.cpp:1167
+
+  assert(Argv.size() == FileStack.back().End);
   return AllExpanded;
----------------
chrisglover wrote:
> smeenai wrote:
> > jyknight wrote:
> > > Worth putting a comment here as to why this is the case.
> > Yeah, this would address the confusion I had :)
> Does this make sense? Too wordy?
> 
>   // If successful, the top of the file stack will mark the end of the Argv
>   // stream. A failure here indicates a bug in the stack popping logic above.
>   // Note that FileStack may have more than one element at this point because we
>   // don't have a chance to pop the stack when encountering recursive files at
>   // the end of the stream, so seeing that doesn't indicate a bug.
> 
> 
SGTM. Probably worth changing the assert to also check size > 0:
assert(FileStack.size() > 0 && Argv.size() == FileStack.back().End);


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

https://reviews.llvm.org/D62798