r262189 - [modules] Prefer more complete array types.
Akira Hatanaka via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 31 16:59:25 PDT 2016
I’ve come up with a patch and sent it to the list today since it is urgent to fix.
https://reviews.llvm.org/D24110 <https://reviews.llvm.org/D24110>
Thank you.
> On Aug 31, 2016, at 1:50 PM, Vassil Vassilev <v.g.vassilev at gmail.com> wrote:
>
> 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 <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 <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 <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 <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 <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 <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 <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 <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 <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/747ec39c/attachment-0001.html>
More information about the cfe-commits
mailing list