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

[Oliver Hunt]

Here goes patch mark 2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-incomplete-array-def-in-extern.patch
Type: application/octet-stream
Size: 3262 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071105/e3464013/attachment.obj>
-------------- next part --------------


--Oliver

On 5/11/2007, at 11:52 AM, Chris Lattner wrote:

>
> 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