Fix clang reject valid C++ code after d999ce0302f06d250f6d496b56a5a5f

[hokein]

The incremental processing mode doesn't seem to work well for C++, see the #89804 (comment) for details.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

The incremental processing mode doesn't seem to work well for C++.

---

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

1 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+2-1)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index a6734ef8c30aa..4b9b735f1cfb4 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2288,7 +2288,8 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
     // Partial translation units that are created in incremental processing must
     // not clean up the IdResolver because PTUs should take into account the
     // declarations that came from previous PTUs.
-    if (!PP.isIncrementalProcessingEnabled() || getLangOpts().ObjC)
+    if (!PP.isIncrementalProcessingEnabled() || getLangOpts().ObjC ||
+        getLangOpts().CPlusPlus)
       IdResolver.RemoveDecl(D);
 
     // Warn on it if we are shadowing a declaration.

[vgvassilev]

LGTM, can you include the produced errors and the steps to reproduce the failure in the commit log? Or refer to the github post describing it?

[hokein]

unfortunately, this seems to break an existing test:

       OK ] InterpreterTest.IncrementalInputTopLevelDecls (66 ms)
[ RUN      ] InterpreterTest.Errors
ClangReplInterpreterTests: /usr/local/google/home/hokein/workspace/llvm-project/clang/lib/Sema/IdentifierResolver.cpp:228: void clang::IdentifierResolver::RemoveDecl(NamedDecl *): Assertion `Ptr && "Didn't find this decl on its identifier's chain!"' failed.
 #0 0x00005591835285e1 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (tools/clang/unittests/Interpreter/ClangReplInterpreterTests+0xa4e45e1)
 #1 0x0000559183528b9b PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #2 0x0000559183526356 llvm::sys::RunSignalHandlers() (tools/clang/unittests/Interpreter/ClangReplInterpreterTests+0xa4e2356)
 #3 0x0000559183529d35 SignalHandler(int) Signals.cpp:0:0
 #4 0x00007f5baf25a510 (/lib/x86_64-linux-gnu/libc.so.6+0x3c510)
 #5 0x00007f5baf2a816

[vgvassilev]

Oh, we need to adjust https://github.com/root-project/root/blob/be5d34934de883270683030b3af2cd1195d17ea8/cmake/modules/RootMacros.cmake#L272 to skip in case of C++...

[hokein]

Oh, we need to adjust https://github.com/root-project/root/blob/be5d34934de883270683030b3af2cd1195d17ea8/cmake/modules/RootMacros.cmake#L272 to skip in case of C++...

The link points to an irrelevant project, I assume you mean here https://github.com/llvm/llvm-project/blob/main/clang/lib/Interpreter/IncrementalParser.cpp#L416? If we need to skip for C++, I think we should do it for ObjectiveC as well, like

if (ND->getDeclName().getFETokenInfo()  &&
    !(!PP.isIncrementalProcessingEnabled() || getLangOpts().ObjC || getLangOpts().CPlusPlus)) {
   ...
}

[vgvassilev]

Oh, we need to adjust https://github.com/root-project/root/blob/be5d34934de883270683030b3af2cd1195d17ea8/cmake/modules/RootMacros.cmake#L272 to skip in case of C++...

The link points to an irrelevant project, I assume you mean here https://github.com/llvm/llvm-project/blob/main/clang/lib/Interpreter/IncrementalParser.cpp#L416? If we need to skip for C++, I think we should do it for ObjectiveC as well, like

if (ND->getDeclName().getFETokenInfo()  &&
    !(!PP.isIncrementalProcessingEnabled() || getLangOpts().ObjC || getLangOpts().CPlusPlus)) {
   ...
}

Not sure how that happened. I meant

llvm-project/clang/lib/Interpreter/IncrementalParser.cpp

Lines 416 to 417 in 70550cd

	if (ND->getDeclName().getFETokenInfo())
	getCI()->getSema().IdResolver.RemoveDecl(ND);

[hokein]

I have update a new version, please take a second look.

[vgvassilev]

Here the incremental processing is guaranteed. We do not need to check.

[hokein]

good point. Removed.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hokein]

I'm landing it now to unblock our integration. I'm happy to address any post comments.