r262189 - [modules] Prefer more complete array types.
Vassil Vassilev via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 31 13:50:21 PDT 2016
Hi Akira,
Thanks for your email. I am on vacation right now and I will try to
look at this issue as soon as I can.
Sorry for the inconvenience.
Cheers,
Vassil
On 31/08/16 22:56, Akira Hatanaka wrote:
> 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 <mailto: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 <mailto: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/90c9759a/attachment-0001.html>
More information about the cfe-commits
mailing list