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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
