Skip to content

[C++20] [Modules] Merge lambdas from Sema to imported one #103036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

ChuanqiXu9
Copy link
Member

Close #102721

Previously we tried to merge lambdas between TUs. But we may still meet problems if we import and then include the headers containing the same lambda.

The solution in this patch is, when we assigned a lambda numbering in a sema, try to see if there is already a same lambda imported. If yes, try to merge them.

Close llvm#102721

Previously we tried to merge lambdas between TUs. But we may still meet
problems if we import and then include the headers containing the same
lambda.

The solution in this patch is, when we assigned a lambda numbering in a
sema, try to see if there is already a same lambda imported. If yes, try
to merge them.
@ChuanqiXu9 ChuanqiXu9 added clang:modules C++20 modules and Clang Header Modules skip-precommit-approval PR for CI feedback, not intended for review labels Aug 13, 2024
@ChuanqiXu9 ChuanqiXu9 self-assigned this Aug 13, 2024
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Aug 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 13, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #102721

Previously we tried to merge lambdas between TUs. But we may still meet problems if we import and then include the headers containing the same lambda.

The solution in this patch is, when we assigned a lambda numbering in a sema, try to see if there is already a same lambda imported. If yes, try to merge them.


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

3 Files Affected:

  • (modified) clang/lib/Serialization/ASTReader.cpp (-11)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+28)
  • (added) clang/test/Modules/pr102721.cppm (+33)
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 511e2df7ad3230..4e4f1780003c5c 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8962,17 +8962,6 @@ void ASTReader::ReadLateParsedTemplates(
   LateParsedTemplates.clear();
 }
 
-void ASTReader::AssignedLambdaNumbering(const CXXRecordDecl *Lambda) {
-  if (Lambda->getLambdaContextDecl()) {
-    // Keep track of this lambda so it can be merged with another lambda that
-    // is loaded later.
-    LambdaDeclarationsForMerging.insert(
-        {{Lambda->getLambdaContextDecl()->getCanonicalDecl(),
-          Lambda->getLambdaIndexInContext()},
-         const_cast<CXXRecordDecl *>(Lambda)});
-  }
-}
-
 void ASTReader::LoadSelector(Selector Sel) {
   // It would be complicated to avoid reading the methods anyway. So don't.
   ReadMethodPool(Sel);
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index a9199f7e50f5dc..c8a0791bb89502 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -4506,6 +4506,34 @@ void ASTReader::loadObjCCategories(GlobalDeclID ID, ObjCInterfaceDecl *D,
   ModuleMgr.visit(Visitor);
 }
 
+void ASTReader::AssignedLambdaNumbering(const CXXRecordDecl *Lambda) {
+  if (Lambda->getLambdaContextDecl()) {
+    auto ContextAndIndexPair = std::make_pair<const Decl *, unsigned>(
+        Lambda->getLambdaContextDecl()->getCanonicalDecl(),
+        Lambda->getLambdaIndexInContext());
+
+    auto Iter = LambdaDeclarationsForMerging.find(ContextAndIndexPair);
+    // If we import first and then include second, try to merge the second
+    // one with the first one.
+    if (Iter != LambdaDeclarationsForMerging.end() &&
+        Iter->second->isFromASTFile()) {
+      NamedDecl *Slot = Iter->second;
+      ASTDeclMerger Merger(*this);
+      // The lambda shouldn't be a key declaration as the lambda wouldn't
+      // be a canonical decl for its owning module.
+      Merger.mergeRedeclarableImpl(const_cast<CXXRecordDecl *>(Lambda),
+                                   cast<TagDecl>(Slot),
+                                   /*KeyDeclID*/ GlobalDeclID());
+      return;
+    }
+
+    // Keep track of this lambda so it can be merged with another lambda that
+    // is loaded later.
+    LambdaDeclarationsForMerging.insert(
+        {ContextAndIndexPair, const_cast<CXXRecordDecl *>(Lambda)});
+  }
+}
+
 template<typename DeclT, typename Fn>
 static void forAllLaterRedecls(DeclT *D, Fn F) {
   F(D);
diff --git a/clang/test/Modules/pr102721.cppm b/clang/test/Modules/pr102721.cppm
new file mode 100644
index 00000000000000..6a84393bcbd1f6
--- /dev/null
+++ b/clang/test/Modules/pr102721.cppm
@@ -0,0 +1,33 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN:   -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++20 %t/test.cc -fsyntax-only -verify \
+// RUN:   -fprebuilt-module-path=%t
+
+//--- foo.h
+inline auto x = []{};
+
+//--- a.cppm
+module;
+#include "foo.h"
+export module a;
+export using ::x;
+
+//--- b.cppm
+module;
+import a;
+#include "foo.h"
+export module b;
+export using ::x;
+
+//--- test.cc
+// expected-no-diagnostics
+import a;
+import b;
+void test() {
+  x();
+}

@ChuanqiXu9 ChuanqiXu9 closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category skip-precommit-approval PR for CI feedback, not intended for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang][Modules] Import then include fails to merge inline lambdas
2 participants