r258524 - Merge templated static member variables, fixes http://llvm.org/pr26179.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 17 17:24:16 PST 2016
+ // 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.vassilev at 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.vassilev at 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.vassilev at 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.vassilev at 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 at 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 at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>
>
More information about the cfe-commits
mailing list