[cfe-commits] PFR: Fix for the issue with incomplete array types in an extern.

[Chris Lattner]

On Nov 4, 2007, at 11:10 PM, Oliver Hunt wrote:

> I'm fairly sure this is correctly formatted, however the style is  
> different from my usual, so it's possible that badness may have  
> slipped through the cracks :-O
>
> <fix-incomplete-array-def-in-extern.patch>
>

This looks great, here are a couple of requests:

+extern int a5[*][2];
+int a5[1][2];

"[*]" is only valid in a function prototype.  We are apparently not  
checking for this yet, but when we do, it will make this test-case  
invalid, please just drop this part.

+extern int n;
+extern int a7[n];

likewise, it isn't valid to define a VLA at global scope.



In the actual code:

-  if (Old->getCanonicalType() != New->getCanonicalType()) {
+  if (Old->getCanonicalType() != New->getCanonicalType() &&
+      !EquivalentArrayTypes(New->getCanonicalType(), Old- 
 >getCanonicalType())) {


The comment for 'equivalentArrayTypes' doesn't say that it requires  
the input types to be canonical.  I'd suggest just allowing any types  
to be passed in and having EquivalentArrayTypes do the  
canonicalization itself.

Also, it looks like you define 'equivalentArrayTypes' but call  
'EquivalentArrayTypes'.


In terms of naming, please name the functions "hasUndefinedLength"  
and "areEquivalentArrayTypes" to emphasize that they are predicates.

+  if (UndefinedLength(NewAT) || UndefinedLength(OldAT)) {
+    if (NewAT->getIndexTypeQualifier() != OldAT- 
 >getIndexTypeQualifier())
+      return false;
+    NewQType = NewAT->getElementType();
+    OldQType = OldAT->getElementType();
+  }

Please put a comment above this block of code explaining what this  
does: maybe give an example?

Thanks Oliver, this is change will fix an often-requested bug!

-Chris