Changeset View
Standalone View
clang/lib/Sema/SemaTemplate.cpp
- This file is larger than 256 KB, so syntax highlighting is disabled by default.
| Show First 20 Lines • Show All 8,649 Lines • ▼ Show 20 Lines | if (NewDecl->hasAssociatedConstraints()) { | ||||||||
| // A concept shall not have associated constraints. | // A concept shall not have associated constraints. | ||||||||
| Diag(NameLoc, diag::err_concept_no_associated_constraints); | Diag(NameLoc, diag::err_concept_no_associated_constraints); | ||||||||
| NewDecl->setInvalidDecl(); | NewDecl->setInvalidDecl(); | ||||||||
| } | } | ||||||||
| // Check for conflicting previous declaration. | // Check for conflicting previous declaration. | ||||||||
| DeclarationNameInfo NameInfo(NewDecl->getDeclName(), NameLoc); | DeclarationNameInfo NameInfo(NewDecl->getDeclName(), NameLoc); | ||||||||
| LookupResult Previous(*this, NameInfo, LookupOrdinaryName, | LookupResult Previous(*this, NameInfo, LookupOrdinaryName, | ||||||||
| ForVisibleRedeclaration); | forRedeclarationInCurContext()); | ||||||||
| LookupName(Previous, S); | LookupName(Previous, S); | ||||||||
| FilterLookupForScope(Previous, DC, S, /*ConsiderLinkage=*/false, | FilterLookupForScope(Previous, DC, S, /*ConsiderLinkage=*/false, | ||||||||
| /*AllowInlineNamespace*/false); | /*AllowInlineNamespace*/false); | ||||||||
| if (!Previous.empty()) { | bool AddToScope = true; | ||||||||
ilya-biryukov: Not sure if reporting the redefinition if `isSameEntity` returns false is the right approach… | |||||||||
Clang tends to do "ambiguous reference" instead of the behavior we had in this patch. ilya-biryukov: Clang tends to do "ambiguous reference" instead of the behavior we had in this patch.
I have… | |||||||||
| auto *Old = Previous.getRepresentativeDecl(); | CheckConceptRedefinition(NewDecl, Previous, AddToScope); | ||||||||
| Diag(NameLoc, isa<ConceptDecl>(Old) ? diag::err_redefinition : | |||||||||
| diag::err_redefinition_different_kind) << NewDecl->getDeclName(); | |||||||||
| Diag(Old->getLocation(), diag::note_previous_definition); | |||||||||
| } | |||||||||
| ActOnDocumentableDecl(NewDecl); | ActOnDocumentableDecl(NewDecl); | ||||||||
| if (AddToScope) | |||||||||
| PushOnScopeChains(NewDecl, S); | PushOnScopeChains(NewDecl, S); | ||||||||
| return NewDecl; | return NewDecl; | ||||||||
| } | } | ||||||||
| void Sema::CheckConceptRedefinition(ConceptDecl *NewDecl, | |||||||||
| LookupResult &Previous, bool &AddToScope) { | |||||||||
| AddToScope = true; | |||||||||
| if (Previous.empty()) | |||||||||
| return; | |||||||||
| // Check if there is a concept declaration we can merge with. | |||||||||
| auto *PrevDef = | |||||||||
| Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr; | |||||||||
| if (PrevDef && !hasVisibleDefinition(PrevDef) && | |||||||||
ChuanqiXu: | |||||||||
I don't think that's right: the C++20 modules rule 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. rsmith: I don't think that's right: [the C++20 modules rule](http://eel.is/c++draft/basic.def.odr#2) is… | |||||||||
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. ChuanqiXu: For clang modules, the reachability should be the visibility: https://github.com/llvm/llvm… | |||||||||
Keeping the reachability for now. Happy to change if we run into trouble later. ilya-biryukov: Keeping the reachability for now. Happy to change if we run into trouble later. | |||||||||
| Context.isSameEntity(NewDecl, PrevDef)) { | |||||||||
| Context.setPrimaryMergedDecl(NewDecl, PrevDef); | |||||||||
| makeMergedDefinitionVisible(PrevDef); | |||||||||
| AddToScope = false; | |||||||||
| return; | |||||||||
| } | |||||||||
| // Filter out non-visible declaration to avoid spurious redefinition errors. | |||||||||
| auto F = Previous.makeFilter(); | |||||||||
| while (F.hasNext()) { | |||||||||
| if (!isVisible(F.next())) { | |||||||||
ChuanqiXu: | |||||||||
| F.erase(); | |||||||||
| } | |||||||||
| } | |||||||||
| F.done(); | |||||||||
| // We report redefinition if the lookup is not empty after filters. | |||||||||
| if (Previous.empty()) | |||||||||
| return; | |||||||||
| auto *Old = Previous.getRepresentativeDecl(); | |||||||||
It'd be nice to have a third diagnostic for the case where there's a known declaration that doesn't match. That is:
rsmith: It'd be nice to have a third diagnostic for the case where there's a known declaration that… | |||||||||
Done. PTAL at the wording for the new error.
I have removed the check visibility of conflicting declarations completely. 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. ilya-biryukov: Done. PTAL at the wording for the new error.
> regardless of whether it's visible or even… | |||||||||
| Diag(NewDecl->getLocation(), isa<ConceptDecl>(Old) | |||||||||
| ? diag::err_redefinition | |||||||||
| : diag::err_redefinition_different_kind) | |||||||||
| << NewDecl->getDeclName(); | |||||||||
| Diag(Old->getLocation(), diag::note_previous_definition); | |||||||||
| } | |||||||||
| /// \brief Strips various properties off an implicit instantiation | /// \brief Strips various properties off an implicit instantiation | ||||||||
| /// that has just been explicitly specialized. | /// that has just been explicitly specialized. | ||||||||
| static void StripImplicitInstantiation(NamedDecl *D) { | static void StripImplicitInstantiation(NamedDecl *D) { | ||||||||
| D->dropAttr<DLLImportAttr>(); | D->dropAttr<DLLImportAttr>(); | ||||||||
| D->dropAttr<DLLExportAttr>(); | D->dropAttr<DLLExportAttr>(); | ||||||||
| if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) | if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) | ||||||||
| FD->setInlineSpecified(false); | FD->setInlineSpecified(false); | ||||||||
| ▲ Show 20 Lines • Show All 2,440 Lines • Show Last 20 Lines | |||||||||
Not sure if reporting the redefinition if isSameEntity returns false is the right approach here.
This leads to errors on definitions of something like:
The alternative is the "ambiguous reference" error on the use of the concept. I will check what happens for variables and typedefs in that case and follow the same pattern.