[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu closed this revision.
ChuanqiXu added a comment.

This is not automated closed by 
https://github.com/llvm/llvm-project/commit/4d9251bd780d20eebbcb124608b36a69787d5575
 due to a typo.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D130614#3685142 , @ilya-biryukov 
wrote:

> Thanks for the thorough explanation and testing.
> LGTM.

Thanks for reviewing!




Comment at: clang/lib/Sema/SemaDecl.cpp:1747
+
+  Module *NewM = New->getOwningModule();
+  Module *OldM = Old->getOwningModule();

ilya-biryukov wrote:
> NIT: maybe accept the modules directly?
I feel like the current style saves more typing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 448504.
ChuanqiXu marked 3 inline comments as done.
ChuanqiXu added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/merge-concepts.cppm

Index: clang/test/Modules/merge-concepts.cppm
===
--- /dev/null
+++ 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 
+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 
+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  void foo()
+  requires same_as
+{}
+
+//--- Use2.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template  void foo()
+  requires same_as
+{}
+
+//--- Use3.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template  void foo()
+  requires same_as
+{}
+
+//--- Use4.cpp
+// expected-no-diagnostics
+import A;
+import B;
+#include "foo.h"
+
+template  void foo()
+  requires same_as
+{}
+
+//--- C.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module C;
+import A;
+import B;
+
+template  void foo()
+  requires same_as
+{}
+
+//--- 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 
+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 
+concept same_as = __is_same(T, U);
+template 
+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  void foo()
+  requires same_as
+{}
+
+//--- Use6.cpp
+// expected-no-diagnostics
+import A;
+import "foo.h";
+
+template  void foo()
+  requires 

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the thorough explanation and testing.
LGTM.




Comment at: clang/lib/Sema/SemaDecl.cpp:1738
+// Return true if the redefinition is not allowed. Return false otherwise.
+bool Sema::CheckRedefinitionInModule(const NamedDecl *New,
+ const NamedDecl *Old) const {

NIT: maybe rename to `IsRedefinitionInModule`?
I would expect `Check...` to produce a diagnostic itself. Having `Is...` in the 
name also removes any confusion about whether the `true` as a return value 
indicates an error or not.




Comment at: clang/lib/Sema/SemaDecl.cpp:1747
+
+  Module *NewM = New->getOwningModule();
+  Module *OldM = Old->getOwningModule();

NIT: maybe accept the modules directly?



Comment at: clang/test/Modules/merge-concepts.cppm:22
+// Testing with module map modules. It is unclear about the relation ship 
between Clang modules and
+// C++20 Named Modules. Will them co_exist? Or will them be mutual from each 
other?
+// The test here is for primarily coverity.

NIT on wording


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added a comment.

BTW, I've sent a NFC test to show the current behavior for functions, classes 
and variables are basically fine: 
https://github.com/llvm/llvm-project/commit/fe1887da36c63f64903de112f2a8e88f973318fa


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added a comment.

In D130614#3681881 , @ilya-biryukov 
wrote:

> Thanks! Mostly questions to better understand the spec and clang. Please 
> excuse me if they sound too basic.
> I have read the modules section of the standard, but I couldn't find where it 
> specifies whether these redefinition errors should be present or not. Any 
> guidance of where to look at?

This is primarily at: http://eel.is/c++draft/basic.def.odr#14:

> [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 different 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, ...
>
> ... (following off is about the ODR checks)

So except the ODR checking, the redefinition errors should be present if:

- The redefinitions lives in one TU. or,
- Any of the redefinitions get attached to a named modules.

So we can get some examples:

---

  // foo.cpp
  template 
  concept same_as = __is_same(T, U);
  template 
  concept same_as = __is_same(T, U);

This is disallowed. Since there are definitions of `same_as` in the same TU for 
`foo.cpp`. Note that headers won't present a TU. So the following one is 
disallowed too:

  // foo.h
  template 
  concept same_as = __is_same(T, U);
  // foo.cpp
  #include "foo.h"
  #include "foo.h"

The definitions lives in the same TU of `foo.cpp` too. So this is problematic.

---

Note that a named module unit (*.cppm files)  presents a TU. So the following 
one is disallowed:

  C++
  // foo.h
  template 
  concept same_as = __is_same(T, U);
  // foo.cppm
  module;
  #include "foo.h"
  #include "foo.h"
  export module foo;

Since there are two definitions in the same TU of `foo.cppm`.

---

And the following one should be allowed:

  C++
  // foo.h
  template 
  concept same_as = __is_same(T, U);
  // foo.cppm
  module;
  #include "foo.h"
  export module foo;
  export using :: same_as;
  // use.cpp
  #include "foo.h"
  import foo;
  template  void foo()
requires same_as
  {}

This is valid since the two definitions lives in two TUs (use.cpp and foo.cppm) 
and neither of them lives in named modules (the `same_as` definition in 
`foo.cppm` lives in global module instead of a named module).

---

And this one is disallowed

  C++
  // foo.h
  template 
  concept same_as = __is_same(T, U);
  // foo.cppm
  export module foo;
  export template 
  concept same_as = __is_same(T, U);
  // use.cpp
  #include "foo.h"
  import foo;
  template  void foo()
requires same_as
  {}

Although the two definitions live in 2 TUs, but one of them is attached to the 
named module `foo`. So it is problematic.

---

Overall, I think the design intuition as follows:
the biggest different of C++20 Named Modules  between clang modules (or the 
standardized one, header units) is that the C++ Named Modules owns a unique TU, 
while the design of clang header modules (or header units) tries to mimic 
headers as much as they can. However, there are full of legacy codes using 
headers. The C++20 Named modules are hard to use in real projects if they can't 
be compatible with existing headers. So here is the global module fragment, 
which is designed for compatibility.
It might be easier to understand the rules now.

I implemented `Sema::CheckRedefinitionInModule` by the way to mimic the rules. 
Although there is a FIXME, I guess the FIXME itself is not too bad. At least it 
won't make things bad : )

> Could we add a few examples where legitimate redefinition errors do happen?

Yeah, added.

> (private module fragment?

In the above wording, there is a requirement `reachable`. But the declarations 
in the private module fragment is not reachable to the users of the module. So 
it won't be a legitimate redefinition error.




Comment at: clang/lib/Sema/SemaTemplate.cpp:8731
 
-  auto *OldConcept = dyn_cast(Previous.getRepresentativeDecl());
+  NamedDecl *Old = Previous.getRepresentativeDecl();
+  if (auto *USD = dyn_cast(Old))

ilya-biryukov wrote:
> This follows what `getFoundDecl()` does, but still allows to show errors when 
> lookup results contain multiple entities.
> 
> A question too: is true that concept is always reachable if it has a 
> reachable using decl?
> I wonder whether `hasReachableDefinition` should be called on 
> `UsingShadowDecl` instead?
> This follows what getFoundDecl() does, but still allows to show errors when 
> lookup results contain multiple entities.

Yeah, this is much better.

> 

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 448246.
ChuanqiXu added a comment.

- Address comment.
- Add Sema::CheckRedefinitionInModule


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/merge-concepts.cppm

Index: clang/test/Modules/merge-concepts.cppm
===
--- /dev/null
+++ 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 them co_exist? Or will them be mutual from each other?
+// 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 
+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 
+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  void foo()
+  requires same_as
+{}
+
+//--- Use2.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template  void foo()
+  requires same_as
+{}
+
+//--- Use3.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template  void foo()
+  requires same_as
+{}
+
+//--- Use4.cpp
+// expected-no-diagnostics
+import A;
+import B;
+#include "foo.h"
+
+template  void foo()
+  requires same_as
+{}
+
+//--- C.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module C;
+import A;
+import B;
+
+template  void foo()
+  requires same_as
+{}
+
+//--- 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 
+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 
+concept same_as = __is_same(T, U);
+template 
+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  void foo()
+  requires same_as
+{}
+
+//--- Use6.cpp
+// expected-no-diagnostics
+import A;
+import "foo.h";
+
+template  void foo()
+  requires 

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks! Mostly questions to better understand the spec and clang. Please excuse 
me if they sound too basic.
I have read the modules section of the standard, but I couldn't find where it 
specifies whether these redefinition errors should be present or not. Any 
guidance of where to look at?

Could we add a few examples where legitimate redefinition errors do happen?
(private module fragment?




Comment at: clang/lib/Sema/SemaTemplate.cpp:8731
 
-  auto *OldConcept = dyn_cast(Previous.getRepresentativeDecl());
+  NamedDecl *Old = Previous.getRepresentativeDecl();
+  if (auto *USD = dyn_cast(Old))

This follows what `getFoundDecl()` does, but still allows to show errors when 
lookup results contain multiple entities.

A question too: is true that concept is always reachable if it has a reachable 
using decl?
I wonder whether `hasReachableDefinition` should be called on `UsingShadowDecl` 
instead?



Comment at: clang/lib/Sema/SemaTemplate.cpp:8755
+  // old one, we should merge them instead of complaining.
+  !(OldConcept->getOwningModule() &&
+OldConcept->getOwningModule()->isGlobalModule())) {

Would we allow redefinitions inside the same global module fragment?
```
module;

template concept ok = true;
template concept ok = true; // should be an error.
```

could we add a test for that?
or do we only mark declarations coming from actual pcm files as having as 
owning module fragment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130614/new/

https://reviews.llvm.org/D130614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added a reviewer: ilya-biryukov.
ChuanqiXu added a project: clang.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a subscriber: cfe-commits.

I found this one when I try to add a C++20 Modules example for D130585 
. Then I found there are something to improve:
(1) When the declaration is shadowed by using decls, we could try to find its 
targeted decl. This is fine since we would check Previous.isSingleResult() 
before we process anything.
(2) When we complain about the redefinition, we need to check for the existence 
of global module fragment. GMF is designed to make C++20 Named modules to be 
compatible with headers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130614

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Modules/merge-concepts-in-GMF.cppm

Index: clang/test/Modules/merge-concepts-in-GMF.cppm
===
--- /dev/null
+++ clang/test/Modules/merge-concepts-in-GMF.cppm
@@ -0,0 +1,63 @@
+// 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 -emit-module-interface -fprebuilt-module-path=%t %t/Use.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/Use2.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/Use3.cpp -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/Use4.cpp -verify -fsyntax-only
+
+//--- foo.h
+template 
+concept same_as = __is_same(T, U);
+
+//--- 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  void foo()
+  requires same_as
+{}
+
+//--- Use2.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+
+template  void foo()
+  requires same_as
+{}
+
+//--- Use3.cpp
+// expected-no-diagnostics
+import A;
+#include "foo.h"
+
+template  void foo()
+  requires same_as
+{}
+
+//--- Use4.cpp
+// expected-no-diagnostics
+import A;
+import B;
+#include "foo.h"
+
+template  void foo()
+  requires same_as
+{}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8728,9 +8728,12 @@
   if (Previous.empty())
 return;
 
-  auto *OldConcept = dyn_cast(Previous.getRepresentativeDecl());
+  NamedDecl *Old = Previous.getRepresentativeDecl();
+  if (auto *USD = dyn_cast(Old))
+Old = USD->getTargetDecl();
+
+  auto *OldConcept = dyn_cast(Old);
   if (!OldConcept) {
-auto *Old = Previous.getRepresentativeDecl();
 Diag(NewDecl->getLocation(), diag::err_redefinition_different_kind)
 << NewDecl->getDeclName();
 notePreviousDefinition(Old, NewDecl->getLocation());
@@ -8746,7 +8749,11 @@
 AddToScope = false;
 return;
   }
-  if (hasReachableDefinition(OldConcept)) {
+  if (hasReachableDefinition(OldConcept) &&
+  // When the old decl lives in the GMF and the new decl is same with the
+  // old one, we should merge them instead of complaining.
+  !(OldConcept->getOwningModule() &&
+OldConcept->getOwningModule()->isGlobalModule())) {
 Diag(NewDecl->getLocation(), diag::err_redefinition)
 << NewDecl->getDeclName();
 notePreviousDefinition(OldConcept, NewDecl->getLocation());
@@ -8758,7 +8765,7 @@
 //Other decls (e.g. namespaces) also have this shortcoming.
 return;
   }
-  Context.setPrimaryMergedDecl(NewDecl, OldConcept);
+  Context.setPrimaryMergedDecl(NewDecl, OldConcept->getCanonicalDecl());
 }
 
 /// \brief Strips various properties off an implicit instantiation
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits