Skip to content

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Aug 8, 2024

When incremental processing is enabled, the Parser will never report tok::eof but tok::annot_repl_input_end. However, that case is already taken care of in IncrementalParser::ParseOrWrapTopLevelDecl() so this check was never executing.

When incremental processing is enabled, the Parser will never report
tok::eof but tok::annot_repl_input_end. However, that case is already
taken care of in IncrementalParser::ParseOrWrapTopLevelDecl() so this
check was never executing.
@hahnjo hahnjo requested a review from vgvassilev August 8, 2024 10:53
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 8, 2024

@llvm/pr-subscribers-clang

Author: Jonas Hahnfeld (hahnjo)

Changes

When incremental processing is enabled, the Parser will never report tok::eof but tok::annot_repl_input_end. However, that case is already taken care of in IncrementalParser::ParseOrWrapTopLevelDecl() so this check was never executing.


Full diff: https://github.com/llvm/llvm-project/pull/102450.diff

1 Files Affected:

  • (modified) clang/lib/Parse/Parser.cpp (-5)
diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index 5ebe71e496a2e8..04c2f1d380bc48 100644
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/lib/Parse/Parser.cpp
@@ -629,11 +629,6 @@ bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
                                Sema::ModuleImportState &ImportState) {
   DestroyTemplateIdAnnotationsRAIIObj CleanupRAII(*this);
 
-  // Skip over the EOF token, flagging end of previous input for incremental
-  // processing
-  if (PP.isIncrementalProcessingEnabled() && Tok.is(tok::eof))
-    ConsumeToken();
-
   Result = nullptr;
   switch (Tok.getKind()) {
   case tok::annot_pragma_unused:

@vgvassilev
Copy link
Contributor

Your reasoning sounds right to me. Can we make sure we are not breaking clang -fincremental-extensions, too?

@hahnjo
Copy link
Member Author

hahnjo commented Aug 8, 2024

Your reasoning sounds right to me. Can we make sure we are not breaking clang -fincremental-extensions, too?

As far as I can tell, -fincremental-extensions should set the language option IncrementalExtensions which in turn is the default for Preprocessor::IncrementalProcessing. The few tests we have with -fincremental-extensions pass with this change.

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@hahnjo hahnjo merged commit 67cb040 into llvm:main Aug 9, 2024
10 checks passed
@hahnjo hahnjo deleted the clang-parser-incremental branch August 9, 2024 07:00
@bnbarham
Copy link
Contributor

As far as I can tell, -fincremental-extensions should set the language option IncrementalExtensions which in turn is the default for Preprocessor::IncrementalProcessing.

It is true that it is the default, but it's possible for clients to have one without the other. This is the case in Swift in particular (see #65683).

The few tests we have with -fincremental-extensions pass with this change.

I couldn't think of a great way to test this at the time. If anyone has any ideas though... 😅. Otherwise we could add a comment here to explain why it's needed?

AnthonyLatsis added a commit to AnthonyLatsis/llvm-project that referenced this pull request Apr 25, 2025
This reverts commit 67cb040.
When importing headers, the Swift ClangImporter library depends on
`clang::Parser::ParseTopLevelDecl` consuming an EOF token when
`IncrementalProcessing` is set.
bnbarham added a commit to swiftlang/llvm-project that referenced this pull request Apr 25, 2025
Revert "[clang] Remove dead incremental Parser code (llvm#102450)"
@hahnjo
Copy link
Member Author

hahnjo commented Apr 29, 2025

As far as I can tell, -fincremental-extensions should set the language option IncrementalExtensions which in turn is the default for Preprocessor::IncrementalProcessing.

It is true that it is the default, but it's possible for clients to have one without the other. This is the case in Swift in particular (see #65683).

The few tests we have with -fincremental-extensions pass with this change.

I couldn't think of a great way to test this at the time. If anyone has any ideas though... 😅. Otherwise we could add a comment here to explain why it's needed?

If I understand correctly, you are manually calling Preprocessor::enableIncrementalProcessing? This should be added as a unittest because right now skipping tokens is not exercised in that scenario... (enableIncrementalProcessing is called in clang/unittests/Frontend/FrontendActionTest.cpp, I missed that at first)

AnthonyLatsis added a commit to swiftlang/llvm-project that referenced this pull request Jun 29, 2025
This reverts commit 67cb040.
When importing headers, the Swift ClangImporter library depends on
`clang::Parser::ParseTopLevelDecl` consuming an EOF token when
`IncrementalProcessing` is set.
AnthonyLatsis added a commit to swiftlang/llvm-project that referenced this pull request Jun 29, 2025
[stable/20250601] Revert "[clang] Remove dead incremental Parser code (llvm#102450)"
AnthonyLatsis added a commit to swiftlang/llvm-project that referenced this pull request Jul 18, 2025
This reverts commit 67cb040.
When importing headers, the Swift ClangImporter library depends on
`clang::Parser::ParseTopLevelDecl` consuming an EOF token when
`IncrementalProcessing` is set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants