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()) { | |||||||||
| auto *Old = | |||||||||
| Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr; | |||||||||
| bool AddToScope = true; | |||||||||
| // Check for redefinition and merge with decl from other module if needed. | |||||||||
| if (Old && !hasVisibleDefinition(Old) && Context.isSameEntity(NewDecl, Old)) { | |||||||||
| Context.setPrimaryMergedDecl(NewDecl, Old); | |||||||||
| makeMergedDefinitionVisible(Old); | |||||||||
| AddToScope = false; | |||||||||
| } else if (!Previous.empty()) { | |||||||||
| auto *Old = Previous.getRepresentativeDecl(); | auto *Old = Previous.getRepresentativeDecl(); | ||||||||
| Diag(NameLoc, isa<ConceptDecl>(Old) ? diag::err_redefinition : | Diag(NameLoc, isa<ConceptDecl>(Old) ? diag::err_redefinition : | ||||||||
ilya-biryukov: Not sure if reporting the redefinition if `isSameEntity` returns false is the right approach… | |||||||||
ilya-biryukovAuthorUnsubmitted 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… | |||||||||
| diag::err_redefinition_different_kind) << NewDecl->getDeclName(); | diag::err_redefinition_different_kind) << NewDecl->getDeclName(); | ||||||||
| Diag(Old->getLocation(), diag::note_previous_definition); | Diag(Old->getLocation(), diag::note_previous_definition); | ||||||||
| } | } | ||||||||
| ActOnDocumentableDecl(NewDecl); | ActOnDocumentableDecl(NewDecl); | ||||||||
| if (AddToScope) | |||||||||
| PushOnScopeChains(NewDecl, S); | PushOnScopeChains(NewDecl, S); | ||||||||
| return NewDecl; | return NewDecl; | ||||||||
| } | } | ||||||||
| /// \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); | ||||||||
| } | } | ||||||||
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. | |||||||||
| /// Compute the diagnostic location for an explicit instantiation | /// Compute the diagnostic location for an explicit instantiation | ||||||||
| // declaration or definition. | // declaration or definition. | ||||||||
| static SourceLocation DiagLocForExplicitInstantiation( | static SourceLocation DiagLocForExplicitInstantiation( | ||||||||
| NamedDecl* D, SourceLocation PointOfInstantiation) { | NamedDecl* D, SourceLocation PointOfInstantiation) { | ||||||||
| // Explicit instantiations following a specialization have no effect and | // Explicit instantiations following a specialization have no effect and | ||||||||
| // hence no PointOfInstantiation. In that case, walk decl backwards | // hence no PointOfInstantiation. In that case, walk decl backwards | ||||||||
| // until a valid name loc is found. | // until a valid name loc is found. | ||||||||
| SourceLocation PrevDiagLoc = PointOfInstantiation; | SourceLocation PrevDiagLoc = PointOfInstantiation; | ||||||||
| for (Decl *Prev = D; Prev && !PrevDiagLoc.isValid(); | for (Decl *Prev = D; Prev && !PrevDiagLoc.isValid(); | ||||||||
ChuanqiXu: | |||||||||
| Prev = Prev->getPreviousDecl()) { | Prev = Prev->getPreviousDecl()) { | ||||||||
| PrevDiagLoc = Prev->getLocation(); | PrevDiagLoc = Prev->getLocation(); | ||||||||
| } | } | ||||||||
| assert(PrevDiagLoc.isValid() && | assert(PrevDiagLoc.isValid() && | ||||||||
| "Explicit instantiation without point of instantiation?"); | "Explicit instantiation without point of instantiation?"); | ||||||||
| return PrevDiagLoc; | return PrevDiagLoc; | ||||||||
| } | } | ||||||||
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… | |||||||||
| /// Diagnose cases where we have an explicit template specialization | /// Diagnose cases where we have an explicit template specialization | ||||||||
| /// before/after an explicit template instantiation, producing diagnostics | /// before/after an explicit template instantiation, producing diagnostics | ||||||||
| /// for those cases where they are required and determining whether the | /// for those cases where they are required and determining whether the | ||||||||
| /// new specialization/instantiation will have any effect. | /// new specialization/instantiation will have any effect. | ||||||||
| /// | /// | ||||||||
| /// \param NewLoc the location of the new explicit specialization or | /// \param NewLoc the location of the new explicit specialization or | ||||||||
| /// instantiation. | /// instantiation. | ||||||||
| /// | /// | ||||||||
| ▲ Show 20 Lines • Show All 2,413 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.