[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8679
+      Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+      Context.isSameEntity(NewDecl, PrevDef)) {
----------------
rsmith wrote:
> ChuanqiXu wrote:
> > 
> I don't think that's right: [the C++20 modules rule](http://eel.is/c++draft/basic.def.odr#2) is that at most one definition is permitted per translation unit, and the Clang header modules rule is that a definition is only permissible if no definition is visible (Clang header modules effectively behave as if they're part of the same translation unit for the purposes of this check). For now, checking for a visible definition seems better than checking for a reachable one, until we implement the C++20 rule in general.
For clang modules, the reachability should be the visibility: https://github.com/llvm/llvm-project/blob/9cb00e7133ea3da6a49ade95e3708240c2aaae39/clang/lib/Sema/SemaLookup.cpp#L1903-L1905. So I think it is better to check for a reachable definition here. Otherwise we might not be able to handle the following case:
```
import M;
export template <class T>
concept ConflictingConcept = true; // There is another reachable but not visible definition for ConflictingConcept.
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921