[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.


================
Comment at: clang/include/clang/Lex/DependencyDirectivesScanner.h:131
+/// \p dependency_directives_scan::tokens_present_before_eof, otherwise this
+/// directive will be ignored.
 ///
----------------
benlangmuir wrote:
> benlangmuir wrote:
> > akyrtzi wrote:
> > > benlangmuir wrote:
> > > > Why would you want to print without this? It seems important for correctness of the output.  I would have expected we would always print it.
> > > The `-print-dependency-directives-minimized-source` clang option is using this function, and if you print the sources with "<TokBEOF>" at the end then the source text will not be parsable.
> > > 
> > > But the tests that pass `-print-dependency-directives-minimized-source` don't try to parse the code, so I think I can switch the default to be `true` for printing the "tokens-before-eof marker" and if a caller has a need to ignore it it can pass `false` for the parameter.
> > If someone uses `-print-dependency-directives-minimized-source` and creates a minimized file for each header, it will "parse", but it won't behave correctly for multiple includes, right?  My preference is we don't allow printing this without the TokBEOF.  If we care about making it parse, we should add a real token of some kind -- maybe there is a no-op `#pragma` or something?
> Oops, missed half my comment. Meant to also add:
> 
> > But the tests that pass -print-dependency-directives-minimized-source don't try to parse the code, so I think I can switch the default to be true for printing the "tokens-before-eof marker" and if a caller has a need to ignore it it can pass false for the parameter.
> 
> SGTM
Ok, you convinced me that we should just always print it. This function is for testing purposes only anyway, if later on we actually have a need to parse back the minimized source we can re-evaluate what to do with that directive marker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133357