This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by akyrtzi on Sep 6 2022, 7:31 AM.

Details

Summary

Directive dependency_directives_scan::tokens_present_before_eof is introduced to indicate there were tokens present before
the last scanned dependency directive and EOF.
This is useful to ensure we correctly identify the macro guards when lexing using the dependency directives.

Diff Detail

Event Timeline

akyrtzi created this revision.Sep 6 2022, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 7:31 AM
Herald added a subscriber: mgorny. · View Herald Transcript
akyrtzi requested review of this revision.Sep 6 2022, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 7:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
akyrtzi updated this revision to Diff 458173.Sep 6 2022, 7:36 AM

Remove a couple of unused local variables.

benlangmuir added inline comments.Sep 6 2022, 9:31 AM
clang/include/clang/Lex/DependencyDirectivesScanner.h
131

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.

clang/lib/Lex/DependencyDirectivesScanner.cpp
868

How about "TokBeforeEOF"? I think "BEOF" is too cryptic.

akyrtzi added inline comments.Sep 6 2022, 2:32 PM
clang/include/clang/Lex/DependencyDirectivesScanner.h
131

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.

clang/lib/Lex/DependencyDirectivesScanner.cpp
868

SGTM 👍

benlangmuir added inline comments.Sep 6 2022, 2:39 PM
clang/include/clang/Lex/DependencyDirectivesScanner.h
131

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?

benlangmuir added inline comments.Sep 6 2022, 2:45 PM
clang/include/clang/Lex/DependencyDirectivesScanner.h
131

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

akyrtzi added inline comments.Sep 6 2022, 4:25 PM
clang/include/clang/Lex/DependencyDirectivesScanner.h
131

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.

akyrtzi updated this revision to Diff 458360.Sep 6 2022, 10:20 PM

Always print tokens_present_before_eof and print it as "<TokBeforeEOF>" for clarity.

akyrtzi marked 4 inline comments as done.Sep 6 2022, 10:24 PM

@benlangmuir see latest diff.

benlangmuir accepted this revision.Sep 7 2022, 7:42 AM

You forgot to remove the \param PrintMarkerForTokensBeforeEOF ... from the doc comment. Otherwise LGTM.

This revision is now accepted and ready to land.Sep 7 2022, 7:42 AM
akyrtzi updated this revision to Diff 458499.Sep 7 2022, 10:20 AM

Remove leftover doc-comment parameter.

This revision was landed with ongoing or failed builds.Sep 7 2022, 10:31 AM
This revision was automatically updated to reflect the committed changes.