[cfe-commits] PFR: Fix for the issue with incomplete array types in an extern.
Oliver Hunt
oliver at apple.com
Mon Nov 5 13:45:30 PST 2007
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
More information about the cfe-commits
mailing list