(And otherwise this LGTM.)
On Wed, Feb 17, 2016 at 5:24 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> + // TODO: 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.
>
> I think this is in the wrong place: it should be attached to the "old
> is complete and new is not" case. Also, our convention is to use
> FIXME, not TODO.
>
> Speaking of which, you can now delete the "FIXME: Even if this merging
> succeeds, [...]" comment now, and add its example as a testcase. With
> your change we should reliably diagnose it.
>
> On Wed, Feb 17, 2016 at 2:32 PM, Vassil Vassilev <v.g.vassi...@gmail.com>
> wrote:
>> On 16/02/16 22:20, Richard Smith wrote:
>>
>> --- a/lib/Sema/SemaLookup.cpp
>> +++ b/lib/Sema/SemaLookup.cpp
>> @@ -419,6 +419,25 @@ static bool isPreferredLookupResult(Sema &S,
>> Sema::LookupNameKind Kind,
>> }
>> }
>>
>> + // 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);
>> + // Skip local VarDecls with incomplete array type.
>> + if ((!DVD->isLocalVarDecl() && DVD->hasExternalStorage()) ||
>> + (!EVD->isLocalVarDecl() && EVD->hasExternalStorage())) {
>>
>> Why do you need this special case?
>>
>> No reason, I decided to be conservative. All comments should be addressed in
>> the new version of the patch.
>>
>> +
>> + ASTContext &C = S.getASTContext();
>> +
>> + if (const ArrayType *DVDTy = C.getAsArrayType(DVD->getType())) {
>> + const ArrayType *EVDTy = C.getAsArrayType(EVD->getType());
>> + // Prefer the decl with more complete type if visible.
>> + return !DVDTy->isIncompleteArrayType() &&
>> EVDTy->isIncompleteArrayType()
>>
>> Checking for an array type here seems unnecessary -- it'd be simpler to
>> check EVD->getType()->isIncompleteType() &&
>> !DVD->getType()->isIncompleteType().
>>
>> + && S.isVisible(D);
>>
>> This seems like a very subtle case: we're performing redeclaration lookup
>> for a variable declaration X, and we find two results D and E. D is hidden
>> but has a complete type. E is visible but has an incomplete type. If X has
>> no array bound, it should not inherit one from D. But if it does have an
>> array bound, and that bound differs from E's bound, we should diagnose the
>> mismatch.
>>
>> Please add another test to the end of merge-incomplete-array-vars.cpp to
>> check this is working:
>> int c[2]; // expected-error {{different type: 'int [2]' vs 'int [1]'}}
>>
>> That said, I think this test will fail if you reorder c1 and c2 in the
>> module map, but that's a pre-existing bug (see the last FIXME in
>> Sema::MergeVarDeclTypes).
>>
>> + }
>> + }
>> + }
>>
>>
>> On Tue, Feb 16, 2016 at 11:12 AM, Vassil Vassilev <v.g.vassi...@gmail.com>
>> wrote:
>>>
>>> ping...
>>>
>>> On 07/02/16 22:33, Vassil Vassilev wrote:
>>>
>>> Improve a comment.
>>> --Vassil
>>> On 07/02/16 20:48, Vassil Vassilev wrote:
>>>
>>> Would this patch be any better?
>>> --Vassil
>>> On 05/02/16 21:49, Richard Smith wrote:
>>>
>>> This belongs in ASTDeclReader::attachPreviousDecl[Impl]. That's where we
>>> propagate data that's supposed to be consistent across a redeclaration chain
>>> from earlier declarations to later ones.
>>>
>>> There's another wrinkle here: we should only be propagating the type from
>>> a previous declaration that's declared in the same scope (in particular, we
>>> should skip over declarations declared as local extern declarations within a
>>> function). This may in some cases require walking backwards along the
>>> redeclaration chain to find the previous declaration that was not a local
>>> extern declaration. That'd be observable in a case like this:
>>>
>>> modulemap:
>>> module A { module A1 { header "a1.h" } module A2 { header "a2.h" } }
>>> module B { header "b.h" }
>>>
>>> a1.h:
>>> int a[];
>>>
>>> b.h:
>>> void g(int(*)[], int);
>>> void g(int(*)[1], int) = delete;
>>> template<typename T> void f(T t) {
>>> extern int a[];
>>> g(&a, t);
>>> }
>>>
>>> a2.h:
>>> int a[1];
>>>
>>> x.cc:
>>> #include "a1.h"
>>> #include "b.h"
>>> void h() { f(0); } // should not produce an error; type of 'a' within 'f'
>>> should not have been updated
>>>
>>> That example actually reveals another problem: we really don't want the
>>> completed type to be visible unless there is a visible declaration with the
>>> completed type. That suggests that propagating the type across the
>>> redeclaration chain may be the wrong approach, and we should instead handle
>>> this by changing isPreferredLookupResult (in SemaLookup.cpp) to prefer a
>>> VarDecl with a complete type over one with an incomplete type.
>>>
>>> On Fri, Feb 5, 2016 at 12:27 PM, Vassil Vassilev <v.g.vassi...@gmail.com>
>>> wrote:
>>>>
>>>> I am not sure where else to put this logic if not in isSameEntity...
>>>> Could you point me to a better place?
>>>> --Vassil
>>>>
>>>> On 05/02/16 19:56, Richard Smith wrote:
>>>>
>>>> On Fri, Feb 5, 2016 at 7:04 AM, Vassil Vassilev <v.g.vassi...@gmail.com>
>>>> wrote:
>>>>>
>>>>> Good point. Do you have in mind calling 'clang::Sema::MergeVarDeclTypes'
>>>>> or we to just "duplicate" the logic in
>>>>> clang::ASTDeclReader::mergeRedeclarable?
>>>>
>>>>
>>>> It's not safe to call into Sema while we're in a partially-deserialized
>>>> state. We know the only interesting case here is complete versus incomplete
>>>> array types, so we don't need anything like the complexity of
>>>> MergeVarDeclTypes -- something as easy as "if (new type is incomplete and
>>>> old type is not) set new type to old type" should work.
>>>>
>>>>>
>>>>> On 05/02/16 02:50, Richard Smith via cfe-commits wrote:
>>>>>
>>>>> I suspect we'll need to do a little more than this: when we rebuild the
>>>>> redeclaration chain, we should probably propagate the complete type to
>>>>> later
>>>>> redeclarations in the same scope. Otherwise, if we import a module that
>>>>> has
>>>>> a complete type and one that has an incomplete type, the declaration found
>>>>> by name lookup might be the one with the incomplete type, possibly
>>>>> resulting
>>>>> in rejects-valid on code like this:
>>>>>
>>>>> a.h:
>>>>> extern int a[5];
>>>>>
>>>>> b.h:
>>>>> extern int a[];
>>>>>
>>>>> x.cc:
>>>>> #include "a.h"
>>>>> #include "b.h"
>>>>> int k = sizeof(a);
>>>>>
>>>>> On Fri, Jan 22, 2016 at 11:03 AM, Yaron Keren via cfe-commits
>>>>> <cfe-commits@lists.llvm.org> wrote:
>>>>>>
>>>>>> Author: yrnkrn
>>>>>> Date: Fri Jan 22 13:03:27 2016
>>>>>> New Revision: 258524
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=258524&view=rev
>>>>>> Log:
>>>>>> Merge templated static member variables, fixes http://llvm.org/pr26179.
>>>>>>
>>>>>> Patch by Vassil Vassilev!
>>>>>> Reviewed by Richard Smith.
>>>>>>
>>>>>>
>>>>>> Added:
>>>>>> cfe/trunk/test/Modules/Inputs/PR26179/
>>>>>> 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
>>>>>> cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
>>>>>> cfe/trunk/test/Modules/pr26179.cpp
>>>>>> Modified:
>>>>>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>>>>
>>>>>> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=258524&r1=258523&r2=258524&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
>>>>>> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Fri Jan 22 13:03:27
>>>>>> 2016
>>>>>> @@ -2595,8 +2595,24 @@ static bool isSameEntity(NamedDecl *X, N
>>>>>> // Variables with the same type and linkage match.
>>>>>> if (VarDecl *VarX = dyn_cast<VarDecl>(X)) {
>>>>>> VarDecl *VarY = cast<VarDecl>(Y);
>>>>>> - return (VarX->getLinkageInternal() == VarY->getLinkageInternal())
>>>>>> &&
>>>>>> - VarX->getASTContext().hasSameType(VarX->getType(),
>>>>>> VarY->getType());
>>>>>> + if (VarX->getLinkageInternal() == VarY->getLinkageInternal()) {
>>>>>> + ASTContext &C = VarX->getASTContext();
>>>>>> + if (C.hasSameType(VarX->getType(), VarY->getType()))
>>>>>> + return true;
>>>>>> +
>>>>>> + // We can get decls with different types on the redecl chain.
>>>>>> Eg.
>>>>>> + // 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.
>>>>>> + const ArrayType *VarXTy = C.getAsArrayType(VarX->getType());
>>>>>> + const ArrayType *VarYTy = C.getAsArrayType(VarY->getType());
>>>>>> + if (!VarXTy || !VarYTy)
>>>>>> + return false;
>>>>>> + if (VarXTy->isIncompleteArrayType() ||
>>>>>> VarYTy->isIncompleteArrayType())
>>>>>> + return C.hasSameType(VarXTy->getElementType(),
>>>>>> VarYTy->getElementType());
>>>>>> + }
>>>>>> + return false;
>>>>>> }
>>>>>>
>>>>>> // Namespaces with the same name and inlinedness match.
>>>>>>
>>>>>> Added: cfe/trunk/test/Modules/Inputs/PR26179/A.h
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/Inputs/PR26179/A.h (added)
>>>>>> +++ cfe/trunk/test/Modules/Inputs/PR26179/A.h Fri Jan 22 13:03:27 2016
>>>>>> @@ -0,0 +1,4 @@
>>>>>> +#include "basic_string.h"
>>>>>> +#include "B.h"
>>>>>> +
>>>>>> +int *p = a;
>>>>>>
>>>>>> Added: cfe/trunk/test/Modules/Inputs/PR26179/B.h
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/Inputs/PR26179/B.h (added)
>>>>>> +++ cfe/trunk/test/Modules/Inputs/PR26179/B.h Fri Jan 22 13:03:27 2016
>>>>>> @@ -0,0 +1,2 @@
>>>>>> +#include "basic_string.h"
>>>>>> +extern int a[5];
>>>>>>
>>>>>> Added: 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=258524&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h (added)
>>>>>> +++ cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h Fri Jan 22
>>>>>> 13:03:27 2016
>>>>>> @@ -0,0 +1,14 @@
>>>>>> +#ifndef _GLIBCXX_STRING
>>>>>> +#define _GLIBCXX_STRING 1
>>>>>> +
>>>>>> +template<typename T>
>>>>>> +struct basic_string {
>>>>>> + static T _S_empty_rep_storage[];
>>>>>> +};
>>>>>> +
>>>>>> +template<typename T>
>>>>>> +T basic_string<T>::_S_empty_rep_storage[sizeof(T)];
>>>>>> +
>>>>>> +extern int a[];
>>>>>> +
>>>>>> +#endif
>>>>>>
>>>>>> Added: cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap (added)
>>>>>> +++ cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap Fri Jan 22
>>>>>> 13:03:27 2016
>>>>>> @@ -0,0 +1,9 @@
>>>>>> +module A {
>>>>>> + header "A.h"
>>>>>> + export *
>>>>>> +}
>>>>>> +
>>>>>> +module B {
>>>>>> + header "B.h"
>>>>>> + export *
>>>>>> +}
>>>>>>
>>>>>> Added: cfe/trunk/test/Modules/pr26179.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/test/Modules/pr26179.cpp (added)
>>>>>> +++ cfe/trunk/test/Modules/pr26179.cpp Fri Jan 22 13:03:27 2016
>>>>>> @@ -0,0 +1,7 @@
>>>>>> +// RUN: rm -rf %t
>>>>>> +// RUN: %clang_cc1 -I%S/Inputs/PR26179 -verify %s
>>>>>> +// RUN: %clang_cc1 -fmodules
>>>>>> -fmodule-map-file=%S/Inputs/PR26179/module.modulemap
>>>>>> -fmodules-cache-path=%t
>>>>>> -I%S/Inputs/PR26179 -verify %s
>>>>>> +
>>>>>> +#include "A.h"
>>>>>> +
>>>>>> +// expected-no-diagnostics
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits@lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits@lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
