[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 <https://reviews.llvm.org/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 <class T, class U>
  concept same_as = __is_same(T, U);
  template <class T, class U>
  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 <class T, class U>
  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 <class T, class U>
  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 <class T, class U>
  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 <class T> void foo()
    requires same_as<T, int>
  {}

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 <class T, class U>
  concept same_as = __is_same(T, U);
  // foo.cppm
  export module foo;
  export template <class T, class U>
  concept same_as = __is_same(T, U);
  // use.cpp
  #include "foo.h"
  import foo;
  template <class T> void foo()
    requires same_as<T, int>
  {}

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<ConceptDecl>(Previous.getRepresentativeDecl());
+  NamedDecl *Old = Previous.getRepresentativeDecl();
+  if (auto *USD = dyn_cast<UsingShadowDecl>(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.

>  is true that concept is always reachable if it has a reachable using decl?

Yes.

> I wonder whether hasReachableDefinition should be called on UsingShadowDecl instead?

We don't need too.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8755
+      // old one, we should merge them instead of complaining.
+      !(OldConcept->getOwningModule() &&
+        OldConcept->getOwningModule()->isGlobalModule())) {
----------------
ilya-biryukov wrote:
> Would we allow redefinitions inside the same global module fragment?
> ```
> module;
> 
> template<class T> concept ok = true;
> template<class T> 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?
I've added this as `D.cppm` in the tests.


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

https://reviews.llvm.org/D130614