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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8694-8696
+  Diag(NewDecl->getLocation(), isa<ConceptDecl>(Old)
+                                   ? diag::err_redefinition
+                                   : diag::err_redefinition_different_kind)
----------------
It'd be nice to have a third diagnostic for the case where there's a known declaration that doesn't match. That is:

* If the name is used for something that's not a concept, diagnose with err_redefinition_different_kind.
* If the name is used for a visible and matching concept, diagnose with err_redefinition.
* If the name is used for a non-matching concept, regardless of whether it's visible or even reachable, produce some new mismatch error.
* Otherwise, if the name is used for a matching concept that's not visible, merge.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8679
+      Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+      Context.isSameEntity(NewDecl, PrevDef)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921