[clang] 98ed0c5 - PR44978: Accept as an extension some cases where destructor name lookup
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 26 14:55:38 PST 2020
Author: Richard Smith
Date: 2020-02-26T14:55:31-08:00
New Revision: 98ed0c5475df57ca5cd4df0997d8bba323c843aa
URL: https://github.com/llvm/llvm-project/commit/98ed0c5475df57ca5cd4df0997d8bba323c843aa
DIFF: https://github.com/llvm/llvm-project/commit/98ed0c5475df57ca5cd4df0997d8bba323c843aa.diff
LOG: PR44978: Accept as an extension some cases where destructor name lookup
is ambiguous, but only one of the possible lookup results could possibly
be right.
Clang recently started diagnosing ambiguity in more cases, and this
broke the build of Firefox. GCC, ICC, MSVC, and previous versions of
Clang all accept some forms of ambiguity here (albeit different ones in
each case); this patch mostly accepts anything any of those compilers
accept.
Added:
Modified:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExprCXX.cpp
clang/test/SemaCXX/destructor.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5cfb12967f14..e22fc9d2232c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1941,6 +1941,9 @@ def ext_dtor_name_missing_template_arguments : Extension<
def ext_qualified_dtor_named_in_lexical_scope : ExtWarn<
"qualified destructor name only found in lexical scope; omit the qualifier "
"to find this type name by unqualified lookup">, InGroup<DtorName>;
+def ext_dtor_name_ambiguous : Extension<
+ "ISO C++ considers this destructor name lookup to be ambiguous">,
+ InGroup<DtorName>;
def err_destroy_attr_on_non_static_var : Error<
"%select{no_destroy|always_destroy}0 attribute can only be applied to a"
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 44fd6997ef3e..c8c6d7c95a7f 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -181,13 +181,23 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc,
ObjectTypePtr ? GetTypeFromParser(ObjectTypePtr) : QualType();
auto CheckLookupResult = [&](LookupResult &Found) -> ParsedType {
- // FIXME: Should we be suppressing ambiguities here?
- if (Found.isAmbiguous()) {
- Failed = true;
- return nullptr;
- }
+ auto IsAcceptableResult = [&](NamedDecl *D) -> bool {
+ auto *Type = dyn_cast<TypeDecl>(D->getUnderlyingDecl());
+ if (!Type)
+ return false;
+
+ if (SearchType.isNull() || SearchType->isDependentType())
+ return true;
+
+ QualType T = Context.getTypeDeclType(Type);
+ return Context.hasSameUnqualifiedType(T, SearchType);
+ };
+ unsigned NumAcceptableResults = 0;
for (NamedDecl *D : Found) {
+ if (IsAcceptableResult(D))
+ ++NumAcceptableResults;
+
// Don't list a class twice in the lookup failure diagnostic if it's
// found by both its injected-class-name and by the name in the enclosing
// scope.
@@ -199,10 +209,34 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc,
FoundDecls.push_back(D);
}
+ // As an extension, attempt to "fix" an ambiguity by erasing all non-type
+ // results, and all non-matching results if we have a search type. It's not
+ // clear what the right behavior is if destructor lookup hits an ambiguity,
+ // but other compilers do generally accept at least some kinds of
+ // ambiguity.
+ if (Found.isAmbiguous() && NumAcceptableResults == 1) {
+ Diag(NameLoc, diag::ext_dtor_name_ambiguous);
+ LookupResult::Filter F = Found.makeFilter();
+ while (F.hasNext()) {
+ NamedDecl *D = F.next();
+ if (auto *TD = dyn_cast<TypeDecl>(D->getUnderlyingDecl()))
+ Diag(D->getLocation(), diag::note_destructor_type_here)
+ << Context.getTypeDeclType(TD);
+ else
+ Diag(D->getLocation(), diag::note_destructor_nontype_here);
+
+ if (!IsAcceptableResult(D))
+ F.erase();
+ }
+ F.done();
+ }
+
+ if (Found.isAmbiguous())
+ Failed = true;
+
if (TypeDecl *Type = Found.getAsSingle<TypeDecl>()) {
- QualType T = Context.getTypeDeclType(Type);
- if (SearchType.isNull() || SearchType->isDependentType() ||
- Context.hasSameUnqualifiedType(T, SearchType)) {
+ if (IsAcceptableResult(Type)) {
+ QualType T = Context.getTypeDeclType(Type);
MarkAnyDeclReferenced(Type->getLocation(), Type, /*OdrUse=*/false);
return CreateParsedType(T,
Context.getTrivialTypeSourceInfo(T, NameLoc));
diff --git a/clang/test/SemaCXX/destructor.cpp b/clang/test/SemaCXX/destructor.cpp
index 7fe1cac3c7e7..bc0048621f76 100644
--- a/clang/test/SemaCXX/destructor.cpp
+++ b/clang/test/SemaCXX/destructor.cpp
@@ -510,4 +510,44 @@ namespace DtorTypedef {
N::C::~C() {}
#pragma clang diagnostic pop
}
+
+// Ignore ambiguity errors in destructor name lookup. This matches the observed
+// behavior of ICC, and is compatible with the observed behavior of GCC (which
+// appears to ignore lookups that result in ambiguity) and MSVC (which appears
+// to perform the lookups in the opposite order from Clang).
+namespace PR44978 {
+ // All compilers accept this despite it being clearly ill-formed per the
+ // current wording.
+ namespace n {
+ class Foo {}; // expected-note {{found}}
+ }
+ class Foo {}; // expected-note {{found}}
+ using namespace n;
+ static void func(n::Foo *p) { p->~Foo(); } // expected-warning {{ambiguous}}
+
+ // GCC rejects this case, ICC accepts, despite the class member lookup being
+ // ambiguous.
+ struct Z;
+ struct X { using T = Z; }; // expected-note {{found}}
+ struct Y { using T = int; }; // expected-note {{found}}
+ struct Z : X, Y {};
+ void f(Z *p) { p->~T(); } // expected-warning {{ambiguous}}
+
+ // GCC accepts this and ignores the ambiguous class member lookup.
+ //
+ // FIXME: We should warn on the ambiguity here too, but that requires us to
+ // keep doing lookups after we've already found the type we want.
+ using T = Z;
+ void g(Z *p) { p->~T(); }
+
+ // ICC accepts this and ignores the ambiguous unqualified lookup.
+ struct Q {};
+ namespace { using U = Q; } // expected-note {{candidate}} expected-note {{found}}
+ using U = int; // expected-note {{candidate}} expected-note {{found}}
+ void f(Q *p) { p->~U(); } // expected-warning {{ambiguous}}
+
+ // We still diagnose if the unqualified lookup is dependent, though.
+ template<typename T> void f(T *p) { p->~U(); } // expected-error {{ambiguous}}
+}
+
#endif // BE_THE_HEADER
More information about the cfe-commits
mailing list