<div dir="ltr"><pre class="">--- 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())) {</pre>Why do you need this special case?<pre class="">+
+ 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()</pre>Checking for an array type here seems unnecessary -- it'd be simpler to check EVD->getType()->isIncompleteType() && !DVD->getType()->isIncompleteType().<pre class="">+ && S.isVisible(D);</pre>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.<div><br></div><div>Please add another test to the end of <span style="color:rgb(0,0,0);font-family:'Courier New',Courier,monospace,arial,sans-serif;font-size:14px;white-space:pre-wrap">merge-incomplete-array-vars.cpp</span> to check this is working:</div><div><font color="#000000" face="Courier New, Courier, monospace, arial, sans-serif"><span style="font-size:14px;white-space:pre-wrap"><br></span></font></div><div><font color="#000000" face="Courier New, Courier, monospace, arial, sans-serif"><span style="font-size:14px;white-space:pre-wrap">int c[2]; // expected-error {{different type: 'int [2]' vs 'int [1]'}}<br></span></font><pre class=""><span style="font-family:arial,sans-serif">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).</span></pre><pre class="">+ }
+ }
+ }</pre></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 16, 2016 at 11:12 AM, Vassil Vassilev <span dir="ltr"><<a href="mailto:v.g.vassilev@gmail.com" target="_blank">v.g.vassilev@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>ping...<div><div class="h5"><br>
On 07/02/16 22:33, Vassil Vassilev wrote:<br>
</div></div></div><div><div class="h5">
<blockquote type="cite">
<div>Improve a comment.<br>
--Vassil<br>
On 07/02/16 20:48, Vassil Vassilev wrote:<br>
</div>
<blockquote type="cite">
<div>Would this patch be any better?<br>
--Vassil<br>
On 05/02/16 21:49, Richard Smith wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">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.
<div><br>
</div>
<div>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:</div>
<div><br>
</div>
<div>modulemap:</div>
<div>module A { module A1 { header "a1.h" } module A2 {
header "a2.h" } }</div>
<div>module B { header "b.h" }</div>
<div><br>
</div>
<div>a1.h:</div>
<div>int a[];</div>
<div><br>
</div>
<div>b.h:</div>
<div>void g(int(*)[], int);</div>
<div>void g(int(*)[1], int) = delete;</div>
<div>template<typename T> void f(T t) {</div>
<div> extern int a[];</div>
<div> g(&a, t);</div>
<div>}</div>
<div><br>
</div>
<div>a2.h:</div>
<div>int a[1];</div>
<div><br>
</div>
<div>x.cc:</div>
<div>#include "a1.h"</div>
<div>#include "b.h"</div>
<div>void h() { f(0); } // should not produce an error; type
of 'a' within 'f' should not have been updated</div>
<div><br>
</div>
<div>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.<br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Feb 5, 2016 at 12:27
PM, Vassil Vassilev <span dir="ltr"><<a href="mailto:v.g.vassilev@gmail.com" target="_blank"></a><a href="mailto:v.g.vassilev@gmail.com" target="_blank">v.g.vassilev@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>I am not sure where else to put this logic if
not in isSameEntity... Could you point me to a
better place?<span><font color="#888888"><br>
--Vassil</font></span>
<div>
<div><br>
On 05/02/16 19:56, Richard Smith wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Fri, Feb 5,
2016 at 7:04 AM, Vassil Vassilev <span dir="ltr"><<a href="mailto:v.g.vassilev@gmail.com" target="_blank"></a><a href="mailto:v.g.vassilev@gmail.com" target="_blank">v.g.vassilev@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Good point. Do you have in
mind calling
'clang::Sema::MergeVarDeclTypes'
or we to just "duplicate" the
logic in
clang::ASTDeclReader::mergeRedeclarable?</div>
</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>
<div>
<div> On 05/02/16 02:50,
Richard Smith via
cfe-commits wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div dir="ltr">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:
<div><br>
</div>
<div>a.h:</div>
<div>extern int a[5];</div>
<div><br>
</div>
<div>b.h:</div>
<div>extern int a[];</div>
<div><br>
</div>
<div>x.cc:</div>
<div>#include "a.h"</div>
<div>#include "b.h"</div>
<div>int k = sizeof(a);</div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On
Fri, Jan 22, 2016 at
11:03 AM, Yaron
Keren via
cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank"></a><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author:
yrnkrn<br>
Date: Fri Jan 22
13:03:27 2016<br>
New Revision:
258524<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=258524&view=rev" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project?rev=258524&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=258524&view=rev</a><br>
Log:<br>
Merge templated
static member
variables, fixes <a href="http://llvm.org/pr26179" target="_blank"></a><a href="http://llvm.org/pr26179" target="_blank">http://llvm.org/pr26179</a>.<br>
<br>
Patch by Vassil
Vassilev!<br>
Reviewed by
Richard Smith.<br>
<br>
<br>
Added:<br>
cfe/trunk/test/Modules/Inputs/PR26179/<br>
cfe/trunk/test/Modules/Inputs/PR26179/A.h<br>
cfe/trunk/test/Modules/Inputs/PR26179/B.h<br>
cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h<br>
cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap<br>
cfe/trunk/test/Modules/pr26179.cpp<br>
Modified:<br>
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
<br>
Modified:
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=258524&r1=258523&r2=258524&view=diff" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=258524&r1=258523&r2=258524&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=258524&r1=258523&r2=258524&view=diff</a><br>
==============================================================================<br>
---
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
(original)<br>
+++
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
Fri Jan 22
13:03:27 2016<br>
@@ -2595,8
+2595,24 @@ static
bool
isSameEntity(NamedDecl
*X, N<br>
// Variables
with the same type
and linkage match.<br>
if (VarDecl
*VarX =
dyn_cast<VarDecl>(X))
{<br>
VarDecl *VarY
=
cast<VarDecl>(Y);<br>
- return
(VarX->getLinkageInternal()
==
VarY->getLinkageInternal())
&&<br>
-
VarX->getASTContext().hasSameType(VarX->getType(),
VarY->getType());<br>
+ if
(VarX->getLinkageInternal()
==
VarY->getLinkageInternal())
{<br>
+ ASTContext
&C =
VarX->getASTContext();<br>
+ if
(C.hasSameType(VarX->getType(),
VarY->getType()))<br>
+ return
true;<br>
+<br>
+ // We can
get decls with
different types on
the redecl chain.
Eg.<br>
+ // template
<typename T>
struct S { static
T Var[]; }; // #1<br>
+ // template
<typename T>
T
S<T>::Var[sizeof(T)];
// #2<br>
+ // Only?
happens when
completing an
incomplete array
type. In this case<br>
+ // when
comparing #1 and
#2 we should go
through their
elements types.<br>
+ const
ArrayType *VarXTy
=
C.getAsArrayType(VarX->getType());<br>
+ const
ArrayType *VarYTy
=
C.getAsArrayType(VarY->getType());<br>
+ if (!VarXTy
|| !VarYTy)<br>
+ return
false;<br>
+ if
(VarXTy->isIncompleteArrayType()
||
VarYTy->isIncompleteArrayType())<br>
+ return
C.hasSameType(VarXTy->getElementType(),
VarYTy->getElementType());<br>
+ }<br>
+ return false;<br>
}<br>
<br>
// Namespaces
with the same name
and inlinedness
match.<br>
<br>
Added:
cfe/trunk/test/Modules/Inputs/PR26179/A.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto</a><br>
==============================================================================<br>
---
cfe/trunk/test/Modules/Inputs/PR26179/A.h
(added)<br>
+++
cfe/trunk/test/Modules/Inputs/PR26179/A.h
Fri Jan 22
13:03:27 2016<br>
@@ -0,0 +1,4 @@<br>
+#include
"basic_string.h"<br>
+#include "B.h"<br>
+<br>
+int *p = a;<br>
<br>
Added:
cfe/trunk/test/Modules/Inputs/PR26179/B.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto</a><br>
==============================================================================<br>
---
cfe/trunk/test/Modules/Inputs/PR26179/B.h
(added)<br>
+++
cfe/trunk/test/Modules/Inputs/PR26179/B.h
Fri Jan 22
13:03:27 2016<br>
@@ -0,0 +1,2 @@<br>
+#include
"basic_string.h"<br>
+extern int a[5];<br>
<br>
Added:
cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h?rev=258524&view=auto" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h?rev=258524&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h?rev=258524&view=auto</a><br>
==============================================================================<br>
---
cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
(added)<br>
+++
cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h
Fri Jan 22
13:03:27 2016<br>
@@ -0,0 +1,14 @@<br>
+#ifndef
_GLIBCXX_STRING<br>
+#define
_GLIBCXX_STRING 1<br>
+<br>
+template<typename
T><br>
+struct
basic_string {<br>
+ static T
_S_empty_rep_storage[];<br>
+};<br>
+<br>
+template<typename
T><br>
+T
basic_string<T>::_S_empty_rep_storage[sizeof(T)];<br>
+<br>
+extern int a[];<br>
+<br>
+#endif<br>
<br>
Added:
cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto</a><br>
==============================================================================<br>
---
cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
(added)<br>
+++
cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap
Fri Jan 22
13:03:27 2016<br>
@@ -0,0 +1,9 @@<br>
+module A {<br>
+ header "A.h"<br>
+ export *<br>
+}<br>
+<br>
+module B {<br>
+ header "B.h"<br>
+ export *<br>
+}<br>
<br>
Added:
cfe/trunk/test/Modules/pr26179.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto" target="_blank"></a><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto</a><br>
==============================================================================<br>
---
cfe/trunk/test/Modules/pr26179.cpp
(added)<br>
+++
cfe/trunk/test/Modules/pr26179.cpp
Fri Jan 22
13:03:27 2016<br>
@@ -0,0 +1,7 @@<br>
+// RUN: rm -rf %t<br>
+// RUN:
%clang_cc1
-I%S/Inputs/PR26179
-verify %s<br>
+// RUN:
%clang_cc1
-fmodules
-fmodule-map-file=%S/Inputs/PR26179/module.modulemap
-fmodules-cache-path=%t
-I%S/Inputs/PR26179
-verify %s<br>
+<br>
+#include "A.h"<br>
+<br>
+//
expected-no-diagnostics<br>
<br>
<br>
_______________________________________________<br>
cfe-commits
mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank"></a><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank"></a><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote>
</div>
<br>
</div>
</div>
</div>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
cfe-commits mailing list
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a>
</pre>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div>