r262189 - [modules] Prefer more complete array types.
Akira Hatanaka via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 31 12:56:33 PDT 2016
Vassil and Richard,
After this commit, clang errors out when compiling the following code, which used to compile without any errors.
$ cat f2.cpp
extern int compile_time_assert_failed[1];
template <typename >
class C {
enum { D };
public:
template <typename A> void foo1() {
extern int compile_time_assert_failed[ ((int)C<A>::k > (int)D) ? 1 : -1];
}
};
template<>
class C<int> {
public:
const static int k = 2;
};
void foo2() {
C<char> c;
c.foo1<int>();
}
$ cat f3.cpp
void foo1() {
extern int foo0[1];
}
template<int n>
void foo2() {
extern int foo0[n ? 1 : -1];
}
void foo3() {
foo2<5>();
}
The code looks legal, so I don’t think clang should complain?
> On Feb 28, 2016, at 11:08 AM, Vassil Vassilev via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>
> Author: vvassilev
> Date: Sun Feb 28 13:08:24 2016
> New Revision: 262189
>
> URL: http://llvm.org/viewvc/llvm-project?rev=262189&view=rev
> Log:
> [modules] Prefer more complete array types.
>
> If we import a module that has a complete array type and one that has an
> incomplete array type, the declaration found by name lookup might be the one with
> the incomplete type, possibly resulting in rejects-valid.
>
> Now, the name lookup prefers decls with a complete array types. Also,
> diagnose cases when the redecl chain has array bound, different from the merge
> candidate.
>
> Reviewed by Richard Smith.
>
> Modified:
> cfe/trunk/lib/Sema/SemaDecl.cpp
> cfe/trunk/lib/Sema/SemaLookup.cpp
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.array/p3.cpp
> cfe/trunk/test/Modules/Inputs/PR26179/A.h
> cfe/trunk/test/Modules/Inputs/PR26179/B.h
> cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Sun Feb 28 13:08:24 2016
> @@ -3245,6 +3245,22 @@ void Sema::mergeObjCMethodDecls(ObjCMeth
> CheckObjCMethodOverride(newMethod, oldMethod);
> }
>
> +static void diagnoseVarDeclTypeMismatch(Sema &S, VarDecl *New, VarDecl* Old) {
> + assert(!S.Context.hasSameType(New->getType(), Old->getType()));
> +
> + S.Diag(New->getLocation(), New->isThisDeclarationADefinition()
> + ? diag::err_redefinition_different_type
> + : diag::err_redeclaration_different_type)
> + << New->getDeclName() << New->getType() << Old->getType();
> +
> + diag::kind PrevDiag;
> + SourceLocation OldLocation;
> + std::tie(PrevDiag, OldLocation)
> + = getNoteDiagForInvalidRedeclaration(Old, New);
> + S.Diag(OldLocation, PrevDiag);
> + New->setInvalidDecl();
> +}
> +
> /// MergeVarDeclTypes - We parsed a variable 'New' which has the same name and
> /// scope as a previous declaration 'Old'. Figure out how to merge their types,
> /// emitting diagnostics as appropriate.
> @@ -3271,21 +3287,40 @@ void Sema::MergeVarDeclTypes(VarDecl *Ne
> // object or function shall be identical, except that declarations for an
> // array object can specify array types that differ by the presence or
> // absence of a major array bound (8.3.4).
> - else if (Old->getType()->isIncompleteArrayType() &&
> - New->getType()->isArrayType()) {
> - const ArrayType *OldArray = Context.getAsArrayType(Old->getType());
> - const ArrayType *NewArray = Context.getAsArrayType(New->getType());
> - if (Context.hasSameType(OldArray->getElementType(),
> - NewArray->getElementType()))
> - MergedT = New->getType();
> - } else if (Old->getType()->isArrayType() &&
> - New->getType()->isIncompleteArrayType()) {
> + else if (Old->getType()->isArrayType() && New->getType()->isArrayType()) {
> const ArrayType *OldArray = Context.getAsArrayType(Old->getType());
> const ArrayType *NewArray = Context.getAsArrayType(New->getType());
> - if (Context.hasSameType(OldArray->getElementType(),
> - NewArray->getElementType()))
> - MergedT = Old->getType();
> - } else if (New->getType()->isObjCObjectPointerType() &&
> +
> + // We are merging a variable declaration New into Old. If it has an array
> + // bound, and that bound differs from Old's bound, we should diagnose the
> + // mismatch.
> + if (!NewArray->isIncompleteArrayType()) {
> + for (VarDecl *PrevVD = Old->getMostRecentDecl(); PrevVD;
> + PrevVD = PrevVD->getPreviousDecl()) {
> + const ArrayType *PrevVDTy = Context.getAsArrayType(PrevVD->getType());
> + if (PrevVDTy->isIncompleteArrayType())
> + continue;
> +
> + if (!Context.hasSameType(NewArray, PrevVDTy))
> + return diagnoseVarDeclTypeMismatch(*this, New, PrevVD);
> + }
> + }
> +
> + if (OldArray->isIncompleteArrayType() && NewArray->isArrayType()) {
> + if (Context.hasSameType(OldArray->getElementType(),
> + NewArray->getElementType()))
> + MergedT = New->getType();
> + }
> + // FIXME: Check visibility. New is hidden but has a complete type. If New
> + // has no array bound, it should not inherit one from Old, if Old is not
> + // visible.
> + else if (OldArray->isArrayType() && NewArray->isIncompleteArrayType()) {
> + if (Context.hasSameType(OldArray->getElementType(),
> + NewArray->getElementType()))
> + MergedT = Old->getType();
> + }
> + }
> + else if (New->getType()->isObjCObjectPointerType() &&
> Old->getType()->isObjCObjectPointerType()) {
> MergedT = Context.mergeObjCGCQualifiers(New->getType(),
> Old->getType());
> @@ -3311,27 +3346,7 @@ void Sema::MergeVarDeclTypes(VarDecl *Ne
> New->setType(Context.DependentTy);
> return;
> }
> -
> - // FIXME: Even if this merging succeeds, some other non-visible declaration
> - // of this variable might have an incompatible type. For instance:
> - //
> - // extern int arr[];
> - // void f() { extern int arr[2]; }
> - // void g() { extern int arr[3]; }
> - //
> - // Neither C nor C++ requires a diagnostic for this, but we should still try
> - // to diagnose it.
> - Diag(New->getLocation(), New->isThisDeclarationADefinition()
> - ? diag::err_redefinition_different_type
> - : diag::err_redeclaration_different_type)
> - << New->getDeclName() << New->getType() << Old->getType();
> -
> - diag::kind PrevDiag;
> - SourceLocation OldLocation;
> - std::tie(PrevDiag, OldLocation) =
> - getNoteDiagForInvalidRedeclaration(Old, New);
> - Diag(OldLocation, PrevDiag);
> - return New->setInvalidDecl();
> + return diagnoseVarDeclTypeMismatch(*this, New, Old);
> }
>
> // Don't actually update the type on the new declaration if the old
>
> Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaLookup.cpp Sun Feb 28 13:08:24 2016
> @@ -419,6 +419,18 @@ static bool isPreferredLookupResult(Sema
> }
> }
>
> + // VarDecl can have incomplete array types, prefer the one with more complete
> + // array type.
> + if (VarDecl *DVD = dyn_cast<VarDecl>(DUnderlying)) {
> + VarDecl *EVD = cast<VarDecl>(EUnderlying);
> + if (EVD->getType()->isIncompleteType() &&
> + !DVD->getType()->isIncompleteType()) {
> + // Prefer the decl with a more complete type if visible.
> + return S.isVisible(DVD);
> + }
> + return false; // Avoid picking up a newer decl, just because it was newer.
> + }
> +
> // For most kinds of declaration, it doesn't really matter which one we pick.
> if (!isa<FunctionDecl>(DUnderlying) && !isa<VarDecl>(DUnderlying)) {
> // If the existing declaration is hidden, prefer the new one. Otherwise,
>
> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Sun Feb 28 13:08:24 2016
> @@ -2613,7 +2613,7 @@ static bool isSameEntity(NamedDecl *X, N
> // template <typename T> struct S { static T Var[]; }; // #1
> // template <typename T> T S<T>::Var[sizeof(T)]; // #2
> // Only? happens when completing an incomplete array type. In this case
> - // when comparing #1 and #2 we should go through their elements types.
> + // when comparing #1 and #2 we should go through their element type.
> const ArrayType *VarXTy = C.getAsArrayType(VarX->getType());
> const ArrayType *VarYTy = C.getAsArrayType(VarY->getType());
> if (!VarXTy || !VarYTy)
>
> Modified: cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.array/p3.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.array/p3.cpp?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.array/p3.cpp (original)
> +++ cfe/trunk/test/CXX/dcl.decl/dcl.meaning/dcl.array/p3.cpp Sun Feb 28 13:08:24 2016
> @@ -207,3 +207,7 @@ namespace use_outside_ns {
> int j() { return sizeof(d); }
> }
> }
> +
> +extern int arr[];
> +void f1() { extern int arr[2]; } // expected-note {{previous}}
> +void f2() { extern int arr[3]; } // expected-error {{different type: 'int [3]' vs 'int [2]'}}
>
> Modified: cfe/trunk/test/Modules/Inputs/PR26179/A.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/PR26179/A.h (original)
> +++ cfe/trunk/test/Modules/Inputs/PR26179/A.h Sun Feb 28 13:08:24 2016
> @@ -1,4 +1,2 @@
> #include "basic_string.h"
> #include "B.h"
> -
> -int *p = a;
>
> Modified: cfe/trunk/test/Modules/Inputs/PR26179/B.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/PR26179/B.h (original)
> +++ cfe/trunk/test/Modules/Inputs/PR26179/B.h Sun Feb 28 13:08:24 2016
> @@ -1,2 +1 @@
> #include "basic_string.h"
> -extern int a[5];
>
> Modified: cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h?rev=262189&r1=262188&r2=262189&view=diff
> ==============================================================================
> --- cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h (original)
> +++ cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h Sun Feb 28 13:08:24 2016
> @@ -9,6 +9,4 @@ struct basic_string {
> template<typename T>
> T basic_string<T>::_S_empty_rep_storage[sizeof(T)];
>
> -extern int a[];
> -
> #endif
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160831/aaad29fa/attachment-0001.html>
More information about the cfe-commits
mailing list