<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Would this patch be any better?<br>
--Vassil<br>
On 05/02/16 21:49, Richard Smith wrote:<br>
</div>
<blockquote
cite="mid:CAOfiQq==e_F=1ZACRu4Z5Wgihouu5KDMTuV5mbeEw1dxzk4Thw@mail.gmail.com"
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 class="HOEnZb"><font color="#888888"><br>
--Vassil</font></span>
<div>
<div class="h5"><br>
On 05/02/16 19:56, Richard Smith wrote:<br>
</div>
</div>
</div>
<div>
<div class="h5">
<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">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
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"
rel="noreferrer"
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"
rel="noreferrer"
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"
rel="noreferrer"
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"
rel="noreferrer"
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"
rel="noreferrer"
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"
rel="noreferrer"
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"
rel="noreferrer"
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"
rel="noreferrer"
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>
</body>
</html>