[Sema] Don't diagnose an array type mismatch when the new or previous declaration has a dependent type

[ahatanak]

Summary

We shouldn't compare the bounds of two arrays and issue a diagnostic if one of them is dependent.

[rsmith]

lib/Sema/SemaDecl.cpp

3370–3374 ↗

(On Diff #69927)

Maybe use isDependentSizedArrayType() here instead, so we still diagnose cases where the element type is dependent but the array bound is non-dependent and differs.

[rsmith]

LGTM as-is, but one suggestion if you want to still allow diagnosing a few more cases.

lib/Sema/SemaDecl.cpp

3377–3378 ↗

(On Diff #69927)

If you do use isDependentSizedArrayType(), you'll need to change this to check the bounds of the array types rather than the type.

[ahatanak]

lib/Sema/SemaDecl.cpp

3377–3378 ↗

(On Diff #69927)

It seems to me that you don't want to do the check when either the array bound or the element type is dependent. Which case would we miss if isDependentType is used here instead of isDependentSizedArrayType? Could you show an example?

[rsmith]

lib/Sema/SemaDecl.cpp

3377–3378 ↗

(On Diff #69927)

Sure. We could diagnose both declarations in the template here:

int a[5];
int b[5];
template<typename T, int N> void f() {
  extern T a[6];
  extern float b[N];
}

... because in both cases the type can never match. However, we don't do this sort of partial type matching in any other cases, so it's very much just a "nice-to-have".

[ahatanak]

lib/Sema/SemaDecl.cpp

3377–3378 ↗

(On Diff #69927)

Ah, I see your point.

If we want to catch those cases too, perhaps we should define and use a function "hasDifferentType" which returns true only if the new and old arrays are known to have different types. In your example, the function would return true for "T a[6]" because we can see it will never match "int a[5]" regardless of what T is.

[ahatanak]

I'll commit the patch as-is first and come up with a new patch which implements the improvements you suggested later.

cfe/trunk/lib/Sema/SemaDecl.cpp

if (getLangOpts().CPlusPlus) {
     //   absence of a major array bound (8.3.4).
     else if (Old->getType()->isArrayType() && New->getType()->isArrayType()) {
       const ArrayType *OldArray = Context.getAsArrayType(Old->getType());
       const ArrayType *NewArray = Context.getAsArrayType(New->getType());
 
       // 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.
-      if (!NewArray->isIncompleteArrayType()) {
+      if (!NewArray->isIncompleteArrayType() && !NewArray->isDependentType()) {
         for (VarDecl *PrevVD = Old->getMostRecentDecl(); PrevVD;
              PrevVD = PrevVD->getPreviousDecl()) {
           const ArrayType *PrevVDTy = Context.getAsArrayType(PrevVD->getType());
-          if (PrevVDTy->isIncompleteArrayType())
+          if (PrevVDTy->isIncompleteArrayType() || PrevVDTy->isDependentType())
             continue;
 
           if (!Context.hasSameType(NewArray, PrevVDTy))
             return diagnoseVarDeclTypeMismatch(*this, New, PrevVD);
         }
       }
 
       if (OldArray->isIncompleteArrayType() && NewArray->isArrayType()) {

cfe/trunk/test/SemaTemplate/array-redeclaration.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+extern int array[1];
+
+template <typename>
+class C {
+  enum { D };
+public:
+  template <typename A> void foo1() {
+    extern int array[((int)C<A>::k > (int)D) ? 1 : -1];
+  }
+};
+
+template<>
+class C<int> {
+public:
+  const static int k = 2;
+};
+
+void foo2() {
+  C<char> c;
+  c.foo1<int>();
+}
+
+template<int n>
+void foo3() {
+  extern int array[n ? 1 : -1];
+}
+
+void foo4() {
+  foo3<5>();
+}