r262189 - [modules] Prefer more complete array types.
Vassil Vassilev via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 31 23:06:18 PDT 2016
On 01/09/16 02:59, Akira Hatanaka wrote:
> 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
>
> Thank you.
I saw it landed in the trunk. Thanks!
>
>> On Aug 31, 2016, at 1:50 PM, Vassil Vassilev <v.g.vassilev at gmail.com
>> <mailto: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
>>>> 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/20160901/4864ac95/attachment-0001.html>
More information about the cfe-commits
mailing list