<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 16/02/16 22:20, Richard Smith wrote:<br>
</div>
<blockquote
cite="mid:CAOfiQqn8=f16oskfa7yXu53tgJjrGPfc0SYS26YLc4FTSmV5HQ@mail.gmail.com"
type="cite">
<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?</div>
</blockquote>
No reason, I decided to be conservative. All comments should be
addressed in the new version of the patch.<br>
<blockquote
cite="mid:CAOfiQqn8=f16oskfa7yXu53tgJjrGPfc0SYS26YLc4FTSmV5HQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<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 face="Courier New, Courier, monospace, arial,
sans-serif" color="#000000"><span style="font-size:14px;white-space:pre-wrap">
</span></font></div>
<div><font face="Courier New, Courier, monospace, arial,
sans-serif" color="#000000"><span style="font-size:14px;white-space:pre-wrap">int c[2]; // expected-error {{different type: 'int [2]' vs 'int [1]'}}
</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 moz-do-not-send="true"
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
moz-do-not-send="true"
href="mailto:v.g.vassilev@gmail.com"
target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:v.g.vassilev@gmail.com">v.g.vassilev@gmail.com</a></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
moz-do-not-send="true"
href="mailto:v.g.vassilev@gmail.com" target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:v.g.vassilev@gmail.com">v.g.vassilev@gmail.com</a></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
moz-do-not-send="true" href="mailto:cfe-commits@lists.llvm.org"
target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project?rev=258524&view=rev"
target="_blank"><a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=258524&view=rev">http://llvm.org/viewvc/llvm-project?rev=258524&view=rev</a></a><br>
Log:<br>
Merge
templated
static member
variables,
fixes <a
moz-do-not-send="true"
href="http://llvm.org/pr26179" target="_blank"><a class="moz-txt-link-freetext" href="http://llvm.org/pr26179">http://llvm.org/pr26179</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=258524&r1=258523&r2=258524&view=diff"
target="_blank"><a class="moz-txt-link-freetext" href="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</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto"
target="_blank"><a class="moz-txt-link-freetext" href="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</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto"
target="_blank"><a class="moz-txt-link-freetext" href="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</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/basic_string.h?rev=258524&view=auto"
target="_blank"><a class="moz-txt-link-freetext" href="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</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto"
target="_blank"><a class="moz-txt-link-freetext" href="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</a></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
moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto"
target="_blank"><a class="moz-txt-link-freetext" href="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</a></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
moz-do-not-send="true"
href="mailto:cfe-commits@lists.llvm.org" target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a></a><br>
<a
moz-do-not-send="true"
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits"
target="_blank"><a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a></a><br>
</blockquote>
</div>
<br>
</div>
</div>
</div>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
cfe-commits mailing list
<a moz-do-not-send="true" href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>
<a moz-do-not-send="true" 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>
</blockquote>
<br>
</body>
</html>