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.vassi...@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.vassi...@gmail.com <mailto:v.g.vassi...@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@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
Log:
Merge templated static member variables, fixes
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
==============================================================================
--- 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
==============================================================================
--- 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
==============================================================================
--- 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
==============================================================================
---
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
==============================================================================
---
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
==============================================================================
--- 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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
From 93c94e46e42dd68597e964a20e17948287e5849d Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassi...@gmail.com>
Date: Fri, 5 Feb 2016 21:15:05 +0100
Subject: [PATCH] [modules] Prefer more complete variable types.
---
lib/Sema/SemaLookup.cpp | 19 +++++++++++++++++++
lib/Serialization/ASTReaderDecl.cpp | 2 +-
test/Modules/Inputs/PR26179/A.h | 2 --
test/Modules/Inputs/PR26179/B.h | 1 -
test/Modules/Inputs/PR26179/basic_string.h | 2 --
.../Modules/Inputs/merge-incomplete-array-vars/a.h | 4 ++++
.../Modules/Inputs/merge-incomplete-array-vars/b.h | 4 ++++
.../Inputs/merge-incomplete-array-vars/c1.h | 1 +
.../Inputs/merge-incomplete-array-vars/c2.h | 1 +
.../Modules/Inputs/merge-incomplete-array-vars/d.h | 7 +++++++
.../merge-incomplete-array-vars/module.modulemap | 5 +++++
test/Modules/merge-incomplete-array-vars.cpp | 22 ++++++++++++++++++++++
12 files changed, 64 insertions(+), 6 deletions(-)
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/a.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/b.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/c1.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/c2.h
create mode 100644 test/Modules/Inputs/merge-incomplete-array-vars/d.h
create mode 100644
test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
create mode 100644 test/Modules/merge-incomplete-array-vars.cpp
diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index 1d7b15c..0d92b62 100644
--- 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())) {
+
+ 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()
+ && S.isVisible(D);
+ }
+ }
+ }
+
// For most kinds of declaration, it doesn't really matter which one we pick.
if (!isa<FunctionDecl>(DUnderlying) && !isa<VarDecl>(DUnderlying)) {
// If the existing declaration is hidden, prefer the new one. Otherwise,
diff --git a/lib/Serialization/ASTReaderDecl.cpp
b/lib/Serialization/ASTReaderDecl.cpp
index 9bcd9a0..feb9d12 100644
--- a/lib/Serialization/ASTReaderDecl.cpp
+++ b/lib/Serialization/ASTReaderDecl.cpp
@@ -2604,7 +2604,7 @@ static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
// 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.
+ // when comparing #1 and #2 we should go through their element type.
const ArrayType *VarXTy = C.getAsArrayType(VarX->getType());
const ArrayType *VarYTy = C.getAsArrayType(VarY->getType());
if (!VarXTy || !VarYTy)
diff --git a/test/Modules/Inputs/PR26179/A.h b/test/Modules/Inputs/PR26179/A.h
index ce95faf..c264f4c 100644
--- a/test/Modules/Inputs/PR26179/A.h
+++ b/test/Modules/Inputs/PR26179/A.h
@@ -1,4 +1,2 @@
#include "basic_string.h"
#include "B.h"
-
-int *p = a;
diff --git a/test/Modules/Inputs/PR26179/B.h b/test/Modules/Inputs/PR26179/B.h
index eb8d1c2..46a109e 100644
--- a/test/Modules/Inputs/PR26179/B.h
+++ b/test/Modules/Inputs/PR26179/B.h
@@ -1,2 +1 @@
#include "basic_string.h"
-extern int a[5];
diff --git a/test/Modules/Inputs/PR26179/basic_string.h
b/test/Modules/Inputs/PR26179/basic_string.h
index afd1e0d..653ce07 100644
--- a/test/Modules/Inputs/PR26179/basic_string.h
+++ b/test/Modules/Inputs/PR26179/basic_string.h
@@ -9,6 +9,4 @@ struct basic_string {
template<typename T>
T basic_string<T>::_S_empty_rep_storage[sizeof(T)];
-extern int a[];
-
#endif
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/a.h
b/test/Modules/Inputs/merge-incomplete-array-vars/a.h
new file mode 100644
index 0000000..4dcf7de
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/a.h
@@ -0,0 +1,4 @@
+extern int a[5];
+extern int aa[55];
+extern int b[];
+extern int bb[];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/b.h
b/test/Modules/Inputs/merge-incomplete-array-vars/b.h
new file mode 100644
index 0000000..8486136
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/b.h
@@ -0,0 +1,4 @@
+extern int a[];
+extern int aa[];
+extern int b[6];
+extern int bb[65];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/c1.h
b/test/Modules/Inputs/merge-incomplete-array-vars/c1.h
new file mode 100644
index 0000000..99be230
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/c1.h
@@ -0,0 +1 @@
+extern int c[];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/c2.h
b/test/Modules/Inputs/merge-incomplete-array-vars/c2.h
new file mode 100644
index 0000000..3fee8c7
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/c2.h
@@ -0,0 +1 @@
+extern int c[1];
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/d.h
b/test/Modules/Inputs/merge-incomplete-array-vars/d.h
new file mode 100644
index 0000000..e24cf76
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/d.h
@@ -0,0 +1,7 @@
+extern int c[];
+void g(int(*)[], int);
+void g(int(*)[1], int) = delete;
+template<typename T> void f(T t) {
+ extern int c[];
+ g(&c, t);
+}
diff --git a/test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
b/test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
new file mode 100644
index 0000000..d2b467d
--- /dev/null
+++ b/test/Modules/Inputs/merge-incomplete-array-vars/module.modulemap
@@ -0,0 +1,5 @@
+module A { header "a.h" export * }
+module B { header "b.h" export * }
+
+module C { module C1 { header "c1.h" } module C2 { header "c2.h" } }
+module D { header "d.h" }
diff --git a/test/Modules/merge-incomplete-array-vars.cpp
b/test/Modules/merge-incomplete-array-vars.cpp
new file mode 100644
index 0000000..4b0c208
--- /dev/null
+++ b/test/Modules/merge-incomplete-array-vars.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/merge-incomplete-array-vars -verify
%s
+// RUN: %clang_cc1 -std=c++11 -fmodules -fimplicit-module-maps
-fmodules-cache-path=%t -I%S/Inputs/merge-incomplete-array-vars -verify %s
+
+
+extern int aa[];
+extern int bb[];
+
+#include "a.h"
+#include "b.h"
+
+// Import the complete type first and the incomplete second, forcing the
+// most recent decl to have an incomplete type.
+int k = sizeof(a);
+int l = sizeof(aa);
+int m = sizeof(b);
+int n = sizeof(bb);
+
+#include "c1.h"
+#include "d.h"
+int size = sizeof(c); // expected-error{{invalid application of 'sizeof' to an
incomplete type 'int []'}}
+void h() { f(0); } // should not produce an error; type of 'c' within 'f'
should not have been updated
--
2.3.8 (Apple Git-58)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
