From 5cd86f8cec1a41b28318ce3a8678e5f53441a753 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 3 Nov 2015 03:13:11 +0000 Subject: [PATCH] [modules] Rationalize the behavior of Decl::declarationReplaces, and in particular don't assume that two declarations of the same kind in the same context are declaring the same entity. That's not true when the same name is declared multiple times as internal-linkage symbols within a module. (getCanonicalDecl is cheap now, so we can just use it here.) llvm-svn: 251898 --- clang/lib/AST/Decl.cpp | 84 +++++++------------ clang/lib/Sema/SemaDecl.cpp | 4 +- clang/lib/Sema/SemaLookup.cpp | 2 +- .../Modules/Inputs/internal-constants/a.h | 3 + .../Modules/Inputs/internal-constants/b.h | 3 + .../Modules/Inputs/internal-constants/c.h | 3 + .../Modules/Inputs/internal-constants/const.h | 3 + .../internal-constants/module.modulemap | 6 ++ clang/test/Modules/internal-constants.cpp | 12 +++ .../cxx1y-variable-templates_top_level.cpp | 4 +- clang/test/SemaCXX/enable_if.cpp | 8 +- 11 files changed, 69 insertions(+), 63 deletions(-) create mode 100644 clang/test/Modules/Inputs/internal-constants/a.h create mode 100644 clang/test/Modules/Inputs/internal-constants/b.h create mode 100644 clang/test/Modules/Inputs/internal-constants/c.h create mode 100644 clang/test/Modules/Inputs/internal-constants/const.h create mode 100644 clang/test/Modules/Inputs/internal-constants/module.modulemap create mode 100644 clang/test/Modules/internal-constants.cpp diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index 098298f0c596..7485b12614d3 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -1451,32 +1451,6 @@ void NamedDecl::getNameForDiagnostic(raw_ostream &OS, printName(OS); } -static bool isKindReplaceableBy(Decl::Kind OldK, Decl::Kind NewK) { - // For method declarations, we never replace. - if (ObjCMethodDecl::classofKind(NewK)) - return false; - - 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; } @@ -1500,9 +1474,19 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const { if (OldD->isFromASTFile() && isFromASTFile()) return false; - if (!isKindReplaceableBy(OldD->getKind(), getKind())) + // A kind mismatch implies that the declaration is not replaced. + if (OldD->getKind() != getKind()) return false; + // For method declarations, we never replace. (Why?) + if (isa(this)) + return false; + + // For parameters, pick the newer one. This is either an error or (in + // Objective-C) permitted as an extension. + if (isa(this)) + 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. @@ -1510,28 +1494,8 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const { 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; - - // 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; - - // Using declarations can be overloaded if they introduce functions. + // Using declarations can be replaced if they import the same name from the + // same context. if (auto *UD = dyn_cast(this)) { ASTContext &Context = getASTContext(); return Context.getCanonicalNestedNameSpecifier(UD->getQualifier()) == @@ -1546,13 +1510,20 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const { } // UsingDirectiveDecl's are not really NamedDecl's, and all have same name. - // We want to keep it, unless it nominates same namespace. + // They can be replaced if they nominate the same namespace. + // FIXME: Is this true even if they have different module visibility? if (auto *UD = dyn_cast(this)) return UD->getNominatedNamespace()->getOriginalNamespace() == cast(OldD)->getNominatedNamespace() ->getOriginalNamespace(); - if (!IsKnownNewer && isRedeclarable(getKind())) { + if (isRedeclarable(getKind())) { + if (getCanonicalDecl() != OldD->getCanonicalDecl()) + return false; + + if (IsKnownNewer) + return true; + // 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. @@ -1567,11 +1538,16 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const { if (D->isCanonicalDecl()) return false; } + + // It's a newer declaration of the same kind of declaration in the same + // scope: we want this decl instead of the existing one. + return true; } - // 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; + // In all other cases, we need to keep both declarations in case they have + // different visibility. Any attempt to use the name will result in an + // ambiguity if more than one is visible. + return false; } bool NamedDecl::hasLinkage() const { diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 622ff1ebbce5..e188094f90ab 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -3352,7 +3352,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) { !TemplateParameterListsAreEqual(NewTemplate->getTemplateParameters(), OldTemplate->getTemplateParameters(), /*Complain=*/true, TPL_TemplateMatch)) - return; + return New->setInvalidDecl(); // C++ [class.mem]p1: // A member shall not be declared twice in the member-specification [...] @@ -8220,7 +8220,7 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD, // there's no more work to do here; we'll just add the new // function to the scope. if (!AllowOverloadingOfFunction(Previous, Context)) { - NamedDecl *Candidate = Previous.getFoundDecl(); + NamedDecl *Candidate = Previous.getRepresentativeDecl(); if (shouldLinkPossiblyHiddenDecl(Candidate, NewFD)) { Redeclaration = true; OldDecl = Candidate; diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp index 6fbe0da8c901..0ba34ea8ef25 100644 --- a/clang/lib/Sema/SemaLookup.cpp +++ b/clang/lib/Sema/SemaLookup.cpp @@ -474,7 +474,7 @@ void LookupResult::resolveKind() { D = cast(D->getCanonicalDecl()); // Ignore an invalid declaration unless it's the only one left. - if (D->isInvalidDecl() && I < N-1) { + if (D->isInvalidDecl() && !(I == 0 && N == 1)) { Decls[I] = Decls[--N]; continue; } diff --git a/clang/test/Modules/Inputs/internal-constants/a.h b/clang/test/Modules/Inputs/internal-constants/a.h new file mode 100644 index 000000000000..d2881381b75d --- /dev/null +++ b/clang/test/Modules/Inputs/internal-constants/a.h @@ -0,0 +1,3 @@ +#pragma once +#include "const.h" +inline int f() { return N::k; } diff --git a/clang/test/Modules/Inputs/internal-constants/b.h b/clang/test/Modules/Inputs/internal-constants/b.h new file mode 100644 index 000000000000..679603afa256 --- /dev/null +++ b/clang/test/Modules/Inputs/internal-constants/b.h @@ -0,0 +1,3 @@ +#pragma once +#include "const.h" +inline int g() { return N::k; } diff --git a/clang/test/Modules/Inputs/internal-constants/c.h b/clang/test/Modules/Inputs/internal-constants/c.h new file mode 100644 index 000000000000..43a37f831a4e --- /dev/null +++ b/clang/test/Modules/Inputs/internal-constants/c.h @@ -0,0 +1,3 @@ +#pragma once +#include "a.h" +inline int h() { return N::k; } diff --git a/clang/test/Modules/Inputs/internal-constants/const.h b/clang/test/Modules/Inputs/internal-constants/const.h new file mode 100644 index 000000000000..e2dc8e14571f --- /dev/null +++ b/clang/test/Modules/Inputs/internal-constants/const.h @@ -0,0 +1,3 @@ +namespace N { + const int k = 5; +} diff --git a/clang/test/Modules/Inputs/internal-constants/module.modulemap b/clang/test/Modules/Inputs/internal-constants/module.modulemap new file mode 100644 index 000000000000..6d471f5fc94f --- /dev/null +++ b/clang/test/Modules/Inputs/internal-constants/module.modulemap @@ -0,0 +1,6 @@ +module X { + textual header "const.h" + module A { header "a.h" export * } + module B { header "b.h" export * } + module C { header "c.h" export * } +} diff --git a/clang/test/Modules/internal-constants.cpp b/clang/test/Modules/internal-constants.cpp new file mode 100644 index 000000000000..f95e95cc8bbb --- /dev/null +++ b/clang/test/Modules/internal-constants.cpp @@ -0,0 +1,12 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodules-local-submodule-visibility -I%S/Inputs/internal-constants %s -verify + +// expected-no-diagnostics +#include "c.h" + +int q = h(); +int r = N::k; + +#include "b.h" + +int s = N::k; // FIXME: This should be ambiguous if we really want internal linkage declarations to not collide. diff --git a/clang/test/SemaCXX/cxx1y-variable-templates_top_level.cpp b/clang/test/SemaCXX/cxx1y-variable-templates_top_level.cpp index 145bc49fff1f..787868fae176 100644 --- a/clang/test/SemaCXX/cxx1y-variable-templates_top_level.cpp +++ b/clang/test/SemaCXX/cxx1y-variable-templates_top_level.cpp @@ -90,8 +90,8 @@ namespace odr_tmpl { } namespace pvt_diff_params { - template T v; // expected-note {{previous template declaration is here}} - template T v; // expected-error {{too few template parameters in template redeclaration}} expected-note {{previous template declaration is here}} + template T v; // expected-note 2{{previous template declaration is here}} + template T v; // expected-error {{too few template parameters in template redeclaration}} template T v; // expected-error {{too many template parameters in template redeclaration}} } diff --git a/clang/test/SemaCXX/enable_if.cpp b/clang/test/SemaCXX/enable_if.cpp index b04d9b4c9dfc..b32bcd01f420 100644 --- a/clang/test/SemaCXX/enable_if.cpp +++ b/clang/test/SemaCXX/enable_if.cpp @@ -11,7 +11,7 @@ struct X { X(bool b) __attribute__((enable_if(b, "chosen when 'b' is true"))); // expected-note{{candidate disabled: chosen when 'b' is true}} void f(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero"))); - void f(int n) __attribute__((enable_if(n == 1, "chosen when 'n' is one"))); // expected-note{{member declaration nearly matches}} expected-note{{candidate disabled: chosen when 'n' is one}} + void f(int n) __attribute__((enable_if(n == 1, "chosen when 'n' is one"))); // expected-note{{member declaration nearly matches}} expected-note 2{{candidate disabled: chosen when 'n' is one}} void g(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero"))); // expected-note{{candidate disabled: chosen when 'n' is zero}} @@ -31,11 +31,11 @@ struct X { operator fp() __attribute__((enable_if(false, "never enabled"))) { return surrogate; } // expected-note{{conversion candidate of type 'int (*)(int)'}} // FIXME: the message is not displayed }; -void X::f(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero"))) // expected-note{{member declaration nearly matches}} expected-note{{candidate disabled: chosen when 'n' is zero}} +void X::f(int n) __attribute__((enable_if(n == 0, "chosen when 'n' is zero"))) // expected-note{{member declaration nearly matches}} expected-note 2{{candidate disabled: chosen when 'n' is zero}} { } -void X::f(int n) __attribute__((enable_if(n == 2, "chosen when 'n' is two"))) // expected-error{{out-of-line definition of 'f' does not match any declaration in 'X'}} expected-note{{candidate disabled: chosen when 'n' is two}} +void X::f(int n) __attribute__((enable_if(n == 2, "chosen when 'n' is two"))) // expected-error{{out-of-line definition of 'f' does not match any declaration in 'X'}} { } @@ -73,7 +73,7 @@ void test() { X x; x.f(0); x.f(1); - x.f(2); // no error, suppressed by erroneous out-of-line definition + x.f(2); // expected-error{{no matching member function for call to 'f'}} x.f(3); // expected-error{{no matching member function for call to 'f'}} x.g(0);