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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
