From 8c75af8aadd460a700db1c25e3df8f133101f6ef Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 10 Feb 2015 03:28:10 +0000 Subject: [PATCH] [modules] When determining whether a name from a module replaces a name we already have, check whether the name from the module is actually newer than the existing declaration. If it isn't, we might (say) replace a visible declaration with an injected friend, and thus make it invisible (or lose a default argument or an array bound). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@228661 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 7 +- include/clang/AST/DeclContextInternals.h | 6 +- lib/AST/Decl.cpp | 150 ++++++++++++------ lib/AST/DeclBase.cpp | 4 +- lib/Sema/SemaLookup.cpp | 5 + .../Inputs/cxx-lookup/module.modulemap | 2 + test/Modules/Inputs/cxx-lookup/na.h | 1 + test/Modules/Inputs/cxx-lookup/nb.h | 1 + test/Modules/cxx-lookup.cpp | 5 + 9 files changed, 123 insertions(+), 58 deletions(-) create mode 100644 test/Modules/Inputs/cxx-lookup/na.h create mode 100644 test/Modules/Inputs/cxx-lookup/nb.h diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index 84ed2862f3..ee2f760830 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -179,14 +179,17 @@ public: const PrintingPolicy &Policy, bool Qualified) const; - /// declarationReplaces - Determine whether this declaration, if + /// \brief Determine whether this declaration, if /// known to be well-formed within its context, will replace the /// declaration OldD if introduced into scope. A declaration will /// replace another declaration if, for example, it is a /// redeclaration of the same variable or function, but not if it is /// a declaration of a different kind (function vs. class) or an /// overloaded function. - bool declarationReplaces(NamedDecl *OldD) const; + /// + /// \param IsKnownNewer \c true if this declaration is known to be newer + /// than \p OldD (for instance, if this declaration is newly-created). + bool declarationReplaces(NamedDecl *OldD, bool IsKnownNewer = true) const; /// \brief Determine whether this declaration has linkage. bool hasLinkage() const; diff --git a/include/clang/AST/DeclContextInternals.h b/include/clang/AST/DeclContextInternals.h index 9068c00a79..e0830d02ae 100644 --- a/include/clang/AST/DeclContextInternals.h +++ b/include/clang/AST/DeclContextInternals.h @@ -163,10 +163,10 @@ public: /// HandleRedeclaration - If this is a redeclaration of an existing decl, /// replace the old one with D and return true. Otherwise return false. - bool HandleRedeclaration(NamedDecl *D) { + bool HandleRedeclaration(NamedDecl *D, bool IsKnownNewer) { // Most decls only have one entry in their list, special case it. if (NamedDecl *OldD = getAsDecl()) { - if (!D->declarationReplaces(OldD)) + if (!D->declarationReplaces(OldD, IsKnownNewer)) return false; setOnlyValue(D); return true; @@ -177,7 +177,7 @@ public: for (DeclsTy::iterator OD = Vec.begin(), ODEnd = Vec.end(); OD != ODEnd; ++OD) { NamedDecl *OldD = *OD; - if (D->declarationReplaces(OldD)) { + if (D->declarationReplaces(OldD, IsKnownNewer)) { *OD = D; return true; } diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 9988c71d97..aadde0cb3a 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -1445,74 +1445,122 @@ void NamedDecl::getNameForDiagnostic(raw_ostream &OS, printName(OS); } -bool NamedDecl::declarationReplaces(NamedDecl *OldD) const { - assert(getDeclName() == OldD->getDeclName() && "Declaration name mismatch"); +static bool isKindReplaceableBy(Decl::Kind OldK, Decl::Kind NewK) { + // For method declarations, we never replace. + if (ObjCMethodDecl::classofKind(NewK)) + return false; - // UsingDirectiveDecl's are not really NamedDecl's, and all have same name. - // We want to keep it, unless it nominates same namespace. - if (getKind() == Decl::UsingDirective) { - return cast(this)->getNominatedNamespace() - ->getOriginalNamespace() == - cast(OldD)->getNominatedNamespace() - ->getOriginalNamespace(); + if (OldK == NewK) + return true; + + // A compatibility alias for a class can be replaced by an interface. + if (ObjCCompatibleAliasDecl::classofKind(OldK) && + ObjCInterfaceDecl::classofKind(NewK)) + return true; + + // A typedef-declaration, alias-declaration, or Objective-C class declaration + // can replace another declaration of the same type. Semantic analysis checks + // that we have matching types. + if ((TypedefNameDecl::classofKind(OldK) || + ObjCInterfaceDecl::classofKind(OldK)) && + (TypedefNameDecl::classofKind(NewK) || + ObjCInterfaceDecl::classofKind(NewK))) + return true; + + // Otherwise, a kind mismatch implies that the declaration is not replaced. + return false; +} + +template static bool isRedeclarableImpl(Redeclarable *) { + return true; +} +static bool isRedeclarableImpl(...) { return false; } +static bool isRedeclarable(Decl::Kind K) { + switch (K) { +#define DECL(Type, Base) \ + case Decl::Type: \ + return isRedeclarableImpl((Type##Decl *)nullptr); +#define ABSTRACT_DECL(DECL) +#include "clang/AST/DeclNodes.inc" } + llvm_unreachable("unknown decl kind"); +} - if (const FunctionDecl *FD = dyn_cast(this)) - // For function declarations, we keep track of redeclarations. - return FD->getPreviousDecl() == OldD; +bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const { + assert(getDeclName() == OldD->getDeclName() && "Declaration name mismatch"); - // For function templates, the underlying function declarations are linked. - if (const FunctionTemplateDecl *FunctionTemplate - = dyn_cast(this)) - if (const FunctionTemplateDecl *OldFunctionTemplate - = dyn_cast(OldD)) - return FunctionTemplate->getTemplatedDecl() - ->declarationReplaces(OldFunctionTemplate->getTemplatedDecl()); - - // For method declarations, we keep track of redeclarations. - if (isa(this)) + if (!isKindReplaceableBy(OldD->getKind(), getKind())) return false; - // FIXME: Is this correct if one of the decls comes from an inline namespace? - if (isa(this) && isa(OldD)) - return true; + // Inline namespaces can give us two declarations with the same + // name and kind in the same scope but different contexts; we should + // keep both declarations in this case. + if (!this->getDeclContext()->getRedeclContext()->Equals( + OldD->getDeclContext()->getRedeclContext())) + return false; + + if (const FunctionDecl *FD = dyn_cast(this)) + // For function declarations, we keep track of redeclarations. + // FIXME: This returns false for functions that should in fact be replaced. + // Instead, perform some kind of type check? + if (FD->getPreviousDecl() != OldD) + return false; - if (isa(this) && isa(OldD)) - return cast(this)->getTargetDecl() == - cast(OldD)->getTargetDecl(); + // For function templates, the underlying function declarations are linked. + if (const FunctionTemplateDecl *FunctionTemplate = + dyn_cast(this)) + return FunctionTemplate->getTemplatedDecl()->declarationReplaces( + cast(OldD)->getTemplatedDecl()); + + // Using shadow declarations can be overloaded on their target declarations + // if they introduce functions. + // FIXME: If our target replaces the old target, can we replace the old + // shadow declaration? + if (auto *USD = dyn_cast(this)) + if (USD->getTargetDecl() != cast(OldD)->getTargetDecl()) + return false; - if (isa(this) && isa(OldD)) { + // Using declarations can be overloaded if they introduce functions. + if (auto *UD = dyn_cast(this)) { ASTContext &Context = getASTContext(); - return Context.getCanonicalNestedNameSpecifier( - cast(this)->getQualifier()) == + return Context.getCanonicalNestedNameSpecifier(UD->getQualifier()) == Context.getCanonicalNestedNameSpecifier( - cast(OldD)->getQualifier()); + cast(OldD)->getQualifier()); } - - if (isa(this) && - isa(OldD)) { + if (auto *UUVD = dyn_cast(this)) { ASTContext &Context = getASTContext(); - return Context.getCanonicalNestedNameSpecifier( - cast(this)->getQualifier()) == + return Context.getCanonicalNestedNameSpecifier(UUVD->getQualifier()) == Context.getCanonicalNestedNameSpecifier( cast(OldD)->getQualifier()); } - // A typedef of an Objective-C class type can replace an Objective-C class - // declaration or definition, and vice versa. - // FIXME: Is this correct if one of the decls comes from an inline namespace? - if ((isa(this) && isa(OldD)) || - (isa(this) && isa(OldD))) - return true; + // UsingDirectiveDecl's are not really NamedDecl's, and all have same name. + // We want to keep it, unless it nominates same namespace. + if (auto *UD = dyn_cast(this)) + return UD->getNominatedNamespace()->getOriginalNamespace() == + cast(OldD)->getNominatedNamespace() + ->getOriginalNamespace(); + + if (!IsKnownNewer && isRedeclarable(getKind())) { + // Check whether this is actually newer than OldD. We want to keep the + // newer declaration. This loop will usually only iterate once, because + // OldD is usually the previous declaration. + for (auto D : redecls()) { + if (D == OldD) + break; - // For non-function declarations, if the declarations are of the - // same kind and have the same parent then this must be a redeclaration, - // or semantic analysis would not have given us the new declaration. - // Note that inline namespaces can give us two declarations with the same - // name and kind in the same scope but different contexts. - return this->getKind() == OldD->getKind() && - this->getDeclContext()->getRedeclContext()->Equals( - OldD->getDeclContext()->getRedeclContext()); + // If we reach the canonical declaration, then OldD is not actually older + // than this one. + // + // FIXME: In this case, we should not add this decl to the lookup table. + if (D->isCanonicalDecl()) + return false; + } + } + + // It's a newer declaration of the same kind of declaration in the same scope, + // and not an overload: we want this decl instead of the existing one. + return true; } bool NamedDecl::hasLinkage() const { diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp index 5f4aa113b7..b4f1c98f71 100644 --- a/lib/AST/DeclBase.cpp +++ b/lib/AST/DeclBase.cpp @@ -1082,7 +1082,7 @@ ExternalASTSource::SetExternalVisibleDeclsForName(const DeclContext *DC, // first. llvm::SmallVector Skip; for (unsigned I = 0, N = Decls.size(); I != N; ++I) - if (List.HandleRedeclaration(Decls[I])) + if (List.HandleRedeclaration(Decls[I], /*IsKnownNewer*/false)) Skip.push_back(I); Skip.push_back(Decls.size()); @@ -1564,7 +1564,7 @@ void DeclContext::makeDeclVisibleInContextImpl(NamedDecl *D, bool Internal) { return; } - if (DeclNameEntries.HandleRedeclaration(D)) { + if (DeclNameEntries.HandleRedeclaration(D, /*IsKnownNewer*/!Internal)) { // This declaration has replaced an existing one for which // declarationReplaces returns true. return; diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index 8edbc1871b..9184a9fd82 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -414,6 +414,8 @@ void LookupResult::resolveKind() { if (!Unique.insert(D).second) { // If it's not unique, pull something off the back (and // continue at this index). + // FIXME: This is wrong. We need to take the more recent declaration in + // order to get the right type, default arguments, etc. Decls[I] = Decls[--N]; continue; } @@ -1254,6 +1256,9 @@ static NamedDecl *findAcceptableDecl(Sema &SemaRef, NamedDecl *D) { for (auto RD : D->redecls()) { if (auto ND = dyn_cast(RD)) { + // FIXME: This is wrong in the case where the previous declaration is not + // visible in the same scope as D. This needs to be done much more + // carefully. if (LookupResult::isVisible(SemaRef, ND)) return ND; } diff --git a/test/Modules/Inputs/cxx-lookup/module.modulemap b/test/Modules/Inputs/cxx-lookup/module.modulemap index 6d397af250..385c8c9b6f 100644 --- a/test/Modules/Inputs/cxx-lookup/module.modulemap +++ b/test/Modules/Inputs/cxx-lookup/module.modulemap @@ -6,3 +6,5 @@ module C { } module X { header "x.h" export * } module Y { header "y.h" export * } +module na { header "na.h" export * } +module nb { header "nb.h" export * } diff --git a/test/Modules/Inputs/cxx-lookup/na.h b/test/Modules/Inputs/cxx-lookup/na.h new file mode 100644 index 0000000000..684d37e8a0 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/na.h @@ -0,0 +1 @@ +namespace N { struct S { friend struct foo; }; } diff --git a/test/Modules/Inputs/cxx-lookup/nb.h b/test/Modules/Inputs/cxx-lookup/nb.h new file mode 100644 index 0000000000..092c882c64 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/nb.h @@ -0,0 +1 @@ +namespace N { extern int n; } diff --git a/test/Modules/cxx-lookup.cpp b/test/Modules/cxx-lookup.cpp index 47c879dbb6..bd019c7f4a 100644 --- a/test/Modules/cxx-lookup.cpp +++ b/test/Modules/cxx-lookup.cpp @@ -4,3 +4,8 @@ namespace llvm {} #include "c2.h" llvm::GlobalValue *p; + +#include "na.h" +namespace N { struct foo; } +#include "nb.h" +N::foo *use_n_foo; -- 2.40.0