[clang] 4d9251b - [C++20] [Modules] Merge same concept decls in global module fragment
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 28 19:51:04 PDT 2022
Author: Chuanqi Xu
Date: 2022-07-29T10:50:27+08:00
New Revision: 4d9251bd780d20eebbcb124608b36a69787d5575
URL: https://github.com/llvm/llvm-project/commit/4d9251bd780d20eebbcb124608b36a69787d5575
DIFF: https://github.com/llvm/llvm-project/commit/4d9251bd780d20eebbcb124608b36a69787d5575.diff
LOG: [C++20] [Modules] Merge same concept decls in global module fragment
According to [basic.def.odr]p14, the same redeclarations in different TU
but not attached to a named module are allowed. But we didn't take care
of concept decl for this condition. This patch tries to fix this
problem.
Reviewed By: ilya-biryukov
Differention Revision: https://reviews.llvm.org/D130614
Added:
clang/test/Modules/merge-concepts.cppm
Modified:
clang/include/clang/Basic/Module.h
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaTemplate.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 47d736a3b4557..8ef1c5991c56e 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -525,6 +525,11 @@ class Module {
Parent->SubModules.push_back(this);
}
+ /// Is this module have similar semantics as headers.
+ bool isHeaderLikeModule() const {
+ return isModuleMapModule() || isHeaderUnit();
+ }
+
/// Is this a module partition.
bool isModulePartition() const {
return Kind == ModulePartitionInterface ||
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 2ada0499ad728..62023fe42a218 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4479,6 +4479,8 @@ class Sema final {
bool CheckRedeclarationModuleOwnership(NamedDecl *New, NamedDecl *Old);
bool CheckRedeclarationExported(NamedDecl *New, NamedDecl *Old);
bool CheckRedeclarationInModule(NamedDecl *New, NamedDecl *Old);
+ bool IsRedefinitionInModule(const NamedDecl *New,
+ const NamedDecl *Old) const;
void DiagnoseAmbiguousLookup(LookupResult &Result);
//@}
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index cc939db89ce6e..31ca96571fc67 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -1719,6 +1719,80 @@ bool Sema::CheckRedeclarationInModule(NamedDecl *New, NamedDecl *Old) {
return false;
}
+// Check the redefinition in C++20 Modules.
+//
+// [basic.def.odr]p14:
+// For any definable item D with definitions in multiple translation units,
+// - if D is a non-inline non-templated function or variable, or
+// - if the definitions in
diff erent translation units do not satisfy the
+// following requirements,
+// the program is ill-formed; a diagnostic is required only if the definable
+// item is attached to a named module and a prior definition is reachable at
+// the point where a later definition occurs.
+// - Each such definition shall not be attached to a named module
+// ([module.unit]).
+// - Each such definition shall consist of the same sequence of tokens, ...
+// ...
+//
+// Return true if the redefinition is not allowed. Return false otherwise.
+bool Sema::IsRedefinitionInModule(const NamedDecl *New,
+ const NamedDecl *Old) const {
+ assert(getASTContext().isSameEntity(New, Old) &&
+ "New and Old are not the same definition, we should diagnostic it "
+ "immediately instead of checking it.");
+ assert(const_cast<Sema *>(this)->isReachable(New) &&
+ const_cast<Sema *>(this)->isReachable(Old) &&
+ "We shouldn't see unreachable definitions here.");
+
+ Module *NewM = New->getOwningModule();
+ Module *OldM = Old->getOwningModule();
+
+ // We only checks for named modules here. The header like modules is skipped.
+ // FIXME: This is not right if we import the header like modules in the module
+ // purview.
+ //
+ // For example, assuming "header.h" provides definition for `D`.
+ // ```C++
+ // //--- M.cppm
+ // export module M;
+ // import "header.h"; // or #include "header.h" but import it by clang modules
+ // actually.
+ //
+ // //--- Use.cpp
+ // import M;
+ // import "header.h"; // or uses clang modules.
+ // ```
+ //
+ // In this case, `D` has multiple definitions in multiple TU (M.cppm and
+ // Use.cpp) and `D` is attached to a named module `M`. The compiler should
+ // reject it. But the current implementation couldn't detect the case since we
+ // don't record the information about the importee modules.
+ //
+ // But this might not be painful in practice. Since the design of C++20 Named
+ // Modules suggests us to use headers in global module fragment instead of
+ // module purview.
+ if (NewM && NewM->isHeaderLikeModule())
+ NewM = nullptr;
+ if (OldM && OldM->isHeaderLikeModule())
+ OldM = nullptr;
+
+ if (!NewM && !OldM)
+ return true;
+
+ // [basic.def.odr]p14.3
+ // Each such definition shall not be attached to a named module
+ // ([module.unit]).
+ if ((NewM && NewM->isModulePurview()) || (OldM && OldM->isModulePurview()))
+ return true;
+
+ // Then New and Old lives in the same TU if their share one same module unit.
+ if (NewM)
+ NewM = NewM->getTopLevelModule();
+ if (OldM)
+ OldM = OldM->getTopLevelModule();
+ return OldM == NewM;
+}
+
static bool isUsingDecl(NamedDecl *D) {
return isa<UsingShadowDecl>(D) ||
isa<UnresolvedUsingTypenameDecl>(D) ||
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index c0bec60f737c8..6f3c5d1847cd8 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -8724,7 +8724,7 @@ void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
if (Previous.empty())
return;
- auto *OldConcept = dyn_cast<ConceptDecl>(Previous.getRepresentativeDecl());
+ auto *OldConcept = dyn_cast<ConceptDecl>(Previous.getRepresentativeDecl()->getUnderlyingDecl());
if (!OldConcept) {
auto *Old = Previous.getRepresentativeDecl();
Diag(NewDecl->getLocation(), diag::err_redefinition_
diff erent_kind)
@@ -8742,7 +8742,8 @@ void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl,
AddToScope = false;
return;
}
- if (hasReachableDefinition(OldConcept)) {
+ if (hasReachableDefinition(OldConcept) &&
+ IsRedefinitionInModule(NewDecl, OldConcept)) {
Diag(NewDecl->getLocation(), diag::err_redefinition)
<< NewDecl->getDeclName();
notePreviousDefinition(OldConcept, NewDecl->getLocation());
diff --git a/clang/test/Modules/merge-concepts.cppm b/clang/test/Modules/merge-concepts.cppm
new file mode 100644
index 0000000000000..ca4877bfd7949
--- /dev/null
+++ b/clang/test/Modules/merge-concepts.cppm
@@ -0,0 +1,185 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/A.cppm -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/B.cppm -o %t/B.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use2.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use3.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use4.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/C.cppm -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -I%t %t/D.cppm -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -I%t %t/E.cppm -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -I%t %t/F.cppm -verify -fsyntax-only
+//
+// Testing header units for coverity.
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/foo.h -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -fmodule-file=%t/foo.pcm %t/Use5.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -fmodule-file=%t/foo.pcm %t/Use6.cpp -verify -fsyntax-only
+//
+// Testing with module map modules. It is unclear about the relation ship between Clang modules and
+// C++20 Named Modules. Will they coexist? Or will they be mutually exclusive?
+// The test here is for primarily coverity.
+//
+// RUN: rm -f %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN: -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN: -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+// Testing module map modules with named modules.
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%t/module.map \
+// RUN: %t/A.cppm -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN: -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -fprebuilt-module-path=%t \
+// RUN: -fmodule-map-file=%t/module.map %t/Use7.cpp -verify -fsyntax-only
+
+//
+//--- foo.h
+#ifndef FOO_H
+#define FOO_H
+template <class T, class U>
+concept same_as = __is_same(T, U);
+#endif
+
+// The compiler would warn if we include foo_h twice without guard.
+//--- redecl.h
+#ifndef REDECL_H
+#define REDECL_H
+template <class T, class U>
+concept same_as = __is_same(T, U);
+#endif
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+export using ::same_as;
+
+//--- B.cppm
+module;
+#include "foo.h"
+export module B;
+export using ::same_as;
+
+//--- Use.cpp
+// expected-no-diagnostics
+import A;
+import B;
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- Use2.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- Use3.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- Use4.cpp
+// expected-no-diagnostics
+import A;
+import B;
+#include "foo.h"
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- C.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module C;
+import A;
+import B;
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- D.cppm
+module;
+#include "foo.h"
+#include "redecl.h"
+export module D;
+export using ::same_as;
+
+// expected-error@* {{redefinition of 'same_as'}}
+// expected-note@* 1+{{previous definition is here}}
+
+//--- E.cppm
+module;
+#include "foo.h"
+export module E;
+export template <class T, class U>
+concept same_as = __is_same(T, U);
+
+// expected-error@* {{redefinition of 'same_as'}}
+// expected-note@* 1+{{previous definition is here}}
+
+//--- F.cppm
+export module F;
+template <class T, class U>
+concept same_as = __is_same(T, U);
+template <class T, class U>
+concept same_as = __is_same(T, U);
+
+// expected-error@* {{redefinition of 'same_as'}}
+// expected-note@* 1+{{previous definition is here}}
+
+//--- Use5.cpp
+// expected-no-diagnostics
+import "foo.h";
+import A;
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- Use6.cpp
+// expected-no-diagnostics
+import A;
+import "foo.h";
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- module.map
+module "foo" {
+ export *
+ header "foo.h"
+}
+
+//--- Use7.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
+
+//--- Use8.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template <class T> void foo()
+ requires same_as<T, int>
+{}
More information about the cfe-commits
mailing list