r258524 - Merge templated static member variables, fixes http://llvm.org/pr26179.

Vassil Vassilev via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 17 14:32:13 PST 2016


On 16/02/16 22:20, Richard Smith wrote:
> --- 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())) {
> Why do you need this special case?
No reason, I decided to be conservative. All comments should be 
addressed in the new version of the patch.
> +
> +      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()
> Checking for an array type here seems unnecessary -- it'd be simpler 
> to check EVD->getType()->isIncompleteType() && 
> !DVD->getType()->isIncompleteType().
> +                 && S.isVisible(D);
> 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.
>
> Please add another test to the end of 
> merge-incomplete-array-vars.cpp to check this is working:
> int c[2]; // expected-error {{different type: 'int [2]' vs 'int [1]'}}
> 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).
> +      }
> +    }
> +  }
>
> On Tue, Feb 16, 2016 at 11:12 AM, Vassil Vassilev 
> <v.g.vassilev at gmail.com <mailto:v.g.vassilev at gmail.com>> wrote:
>
>     ping...
>
>     On 07/02/16 22:33, Vassil Vassilev wrote:
>>     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.vassilev at gmail.com <mailto:v.g.vassilev at 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.vassilev at gmail.com <mailto:v.g.vassilev at 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 at lists.llvm.org
>>>>>>             <mailto:cfe-commits at 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 at lists.llvm.org
>>>>>>                 <mailto:cfe-commits at lists.llvm.org>
>>>>>>                 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>             _______________________________________________
>>>>>>             cfe-commits mailing list
>>>>>>             cfe-commits at lists.llvm.org
>>>>>>             <mailto:cfe-commits at lists.llvm.org>
>>>>>>             http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160218/dc4fa9bb/attachment-0001.html>
-------------- next part --------------
From 9d32e9590135345c2d3e3fdea31c1850720244b2 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev at gmail.com>
Date: Fri, 5 Feb 2016 21:15:05 +0100
Subject: [PATCH] [modules] Prefer more complete variable types.

---
 lib/Sema/SemaDecl.cpp                              | 74 +++++++++++++++-------
 lib/Sema/SemaLookup.cpp                            | 12 ++++
 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       | 21 ++++++
 13 files changed, 106 insertions(+), 30 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/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 66c0e05..4e999d3 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -3245,6 +3245,22 @@ void Sema::mergeObjCMethodDecls(ObjCMethodDecl *newMethod,
   CheckObjCMethodOverride(newMethod, oldMethod);
 }
 
+static void diagnoseVarDeclTypeMismatch(Sema &S, VarDecl *New, VarDecl* Old) {
+  assert(!S.Context.hasSameType(New->getType(), Old->getType()));
+
+  S.Diag(New->getLocation(), New->isThisDeclarationADefinition()
+         ? diag::err_redefinition_different_type
+         : diag::err_redeclaration_different_type)
+    << New->getDeclName() << New->getType() << Old->getType();
+
+  diag::kind PrevDiag;
+  SourceLocation OldLocation;
+  std::tie(PrevDiag, OldLocation)
+    = getNoteDiagForInvalidRedeclaration(Old, New);
+  S.Diag(OldLocation, PrevDiag);
+  New->setInvalidDecl();
+}
+
 /// MergeVarDeclTypes - We parsed a variable 'New' which has the same name and
 /// scope as a previous declaration 'Old'.  Figure out how to merge their types,
 /// emitting diagnostics as appropriate.
@@ -3271,21 +3287,40 @@ void Sema::MergeVarDeclTypes(VarDecl *New, VarDecl *Old,
     //   object or function shall be identical, except that declarations for an
     //   array object can specify array types that differ by the presence or
     //   absence of a major array bound (8.3.4).
-    else if (Old->getType()->isIncompleteArrayType() &&
-             New->getType()->isArrayType()) {
-      const ArrayType *OldArray = Context.getAsArrayType(Old->getType());
-      const ArrayType *NewArray = Context.getAsArrayType(New->getType());
-      if (Context.hasSameType(OldArray->getElementType(),
-                              NewArray->getElementType()))
-        MergedT = New->getType();
-    } else if (Old->getType()->isArrayType() &&
-               New->getType()->isIncompleteArrayType()) {
+    else if (Old->getType()->isArrayType() && New->getType()->isArrayType()) {
       const ArrayType *OldArray = Context.getAsArrayType(Old->getType());
       const ArrayType *NewArray = Context.getAsArrayType(New->getType());
-      if (Context.hasSameType(OldArray->getElementType(),
-                              NewArray->getElementType()))
-        MergedT = Old->getType();
-    } else if (New->getType()->isObjCObjectPointerType() &&
+
+      // We are merging a variable declaration New into Old. If it has an array
+      // bound, and that bound differs from Old's bound, we should diagnose the
+      // mismatch.
+      // TODO: Check visibility. New is hidden but has a complete type. If New
+      // has no array bound, it should not inherit one from Old, if Old is not
+      // visible.
+      if (!NewArray->isIncompleteArrayType()) {
+        for (VarDecl *PrevVD = Old->getMostRecentDecl(); PrevVD;
+             PrevVD = PrevVD->getPreviousDecl()) {
+          const ArrayType *PrevVDTy = Context.getAsArrayType(PrevVD->getType());
+          if (PrevVDTy->isIncompleteArrayType())
+            continue;
+
+          if (!Context.hasSameType(NewArray, PrevVDTy))
+            return diagnoseVarDeclTypeMismatch(*this, New, PrevVD);
+        }
+      }
+
+      if (OldArray->isIncompleteArrayType() && NewArray->isArrayType()) {
+        if (Context.hasSameType(OldArray->getElementType(),
+                                NewArray->getElementType()))
+          MergedT = New->getType();
+      }
+      else if (OldArray->isArrayType() && NewArray->isIncompleteArrayType()) {
+        if (Context.hasSameType(OldArray->getElementType(),
+                                NewArray->getElementType()))
+          MergedT = Old->getType();
+      }
+    }
+    else if (New->getType()->isObjCObjectPointerType() &&
                Old->getType()->isObjCObjectPointerType()) {
       MergedT = Context.mergeObjCGCQualifiers(New->getType(),
                                               Old->getType());
@@ -3321,17 +3356,8 @@ void Sema::MergeVarDeclTypes(VarDecl *New, VarDecl *Old,
     //
     // Neither C nor C++ requires a diagnostic for this, but we should still try
     // to diagnose it.
-    Diag(New->getLocation(), New->isThisDeclarationADefinition()
-                                 ? diag::err_redefinition_different_type
-                                 : diag::err_redeclaration_different_type)
-        << New->getDeclName() << New->getType() << Old->getType();
-
-    diag::kind PrevDiag;
-    SourceLocation OldLocation;
-    std::tie(PrevDiag, OldLocation) =
-        getNoteDiagForInvalidRedeclaration(Old, New);
-    Diag(OldLocation, PrevDiag);
-    return New->setInvalidDecl();
+
+    return diagnoseVarDeclTypeMismatch(*this, New, Old);
   }
 
   // Don't actually update the type on the new declaration if the old
diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index 1d7b15c..7405032 100644
--- a/lib/Sema/SemaLookup.cpp
+++ b/lib/Sema/SemaLookup.cpp
@@ -419,6 +419,18 @@ 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);
+    if (EVD->getType()->isIncompleteType() &&
+        !DVD->getType()->isIncompleteType()) {
+      // Prefer the decl with a more complete type if visible.
+      return S.isVisible(DVD);
+    }
+    return false; // Avoid picking up a newer decl, just because it was newer.
+  }
+
   // 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..dd08b64
--- /dev/null
+++ b/test/Modules/merge-incomplete-array-vars.cpp
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// 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
+int c[2]; // expected-error {{different type: 'int [2]' vs 'int [1]'}} //expected-note at c2.h:1 {{previous declaration is here}}
-- 
2.3.8 (Apple Git-58)



More information about the cfe-commits mailing list