[PATCH] D62798: Explicitly detect recursive response files

[Chris Glover via Phabricator via llvm-commits]

chrisglover marked 4 inline comments as done.
chrisglover added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1106
+
+  SmallVector<ResponseFileRecord, 3> FileStack;
+
----------------
jyknight wrote:
> Could use a comment as to what this represents -- the position of the last argument that came (including recursively) from the named file.
How about

  // To detect recursive response files, we maintain a stack of files and the
  // position of the last argument in the file. This position is
  // updated dynamically as we recursively expand files.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1114
   for (unsigned I = 0; I != Argv.size();) {
+    while (I == FileStack.back().End) {
+      FileStack.pop_back();
----------------
jyknight wrote:
> Also can use a comment -- drop any files from the stack if we've finished their arguments.
How about
     
   // Passing the end of a file's argument list, so we can remove it from the
   // stack.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1167
+
+  assert(Argv.size() == FileStack.back().End);
   return AllExpanded;
----------------
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.




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

https://reviews.llvm.org/D62798