r258524 - Merge templated static member variables, fixes http://llvm.org/pr26179.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 16 12:20:10 PST 2016
--- 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?
+
+ 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>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>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>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>
>>>> http://llvm.org/viewvc/llvm-project?rev=258524&view=rev
>>>> Log:
>>>> Merge templated static member variables, fixes
>>>> <http://llvm.org/pr26179>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>
>>>> 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>
>>>> 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>
>>>> 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>
>>>> 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>
>>>> 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>
>>>> 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>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
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing listcfe-commits at lists.llvm.orghttp://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/20160216/d60cdb8d/attachment-0001.html>
More information about the cfe-commits
mailing list