<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Improve a comment.<br>
--Vassil<br>
On 07/02/16 20:48, Vassil Vassilev wrote:<br>
</div>
<blockquote cite="mid:56B79F80.8030105@gmail.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<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"
class="moz-txt-link-abbreviated"
href="mailto:v.g.vassilev@gmail.com"><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"><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"
class="moz-txt-link-abbreviated"
href="mailto:cfe-commits@lists.llvm.org"><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"
class="moz-txt-link-freetext"
href="http://llvm.org/viewvc/llvm-project?rev=258524&view=rev"><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" class="moz-txt-link-freetext"
href="http://llvm.org/pr26179"><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"
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"><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"
class="moz-txt-link-freetext"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/A.h?rev=258524&view=auto"><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"
class="moz-txt-link-freetext"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/B.h?rev=258524&view=auto"><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"
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"><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"
class="moz-txt-link-freetext"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/PR26179/module.modulemap?rev=258524&view=auto"><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"
class="moz-txt-link-freetext"
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/pr26179.cpp?rev=258524&view=auto"><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"
class="moz-txt-link-abbreviated"
href="mailto:cfe-commits@lists.llvm.org"><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"
class="moz-txt-link-freetext"
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits"><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>
</body>
</html>