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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.

Sorry, was out again due to an emergency. So I wanted to give a chance for any last comments despite having an LGTM
I plan to land this on Monday.



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8694-8696
+  Diag(NewDecl->getLocation(), isa<ConceptDecl>(Old)
+                                   ? diag::err_redefinition
+                                   : diag::err_redefinition_different_kind)
----------------
rsmith wrote:
> 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.
Done. PTAL at the wording for the new error.

> regardless of whether it's visible or even reachable, produce some new mismatch error.
I have removed the check visibility of conflicting declarations completely.
I assume it does not make sense to distinguish between symbols of the same kind or different kinds as both are ODR violations, right?

There is a corner case where we don't report a conflict now: lookup with >1 result and a matching non-visible concept as a representative decl. I did not find a good error for this case, and there are other symbols that have this problem, e.g. namespaces.

It's not easy to trigger this, but I bet it's possible. 


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8679
+      Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+      Context.isSameEntity(NewDecl, PrevDef)) {
----------------
ChuanqiXu wrote:
> 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.
> ```
> 
Keeping the reachability for now. Happy to change if we run into trouble later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921