[cfe-dev] Unneeded declarations invalidation

[Abramo Bagnara]

Douglas Gregor ha scritto:
> On Jul 7, 2009, at 12:48 AM, Enea Zaffanella wrote:
> 
>> Eli Friedman wrote:
>>> On Thu, Jul 2, 2009 at 1:30 AM, Abramo  
>>> Bagnara<abramobagnara at tin.it> wrote:
>>>> The following typescript show what happens when trying to compile a
>>>> simple C source not legal for C99 standard, but accepted and  
>>>> compiled by
>>>> gcc.
>> [...SNIP...]
>>
>>>> OTOH in both cases, I'd like to avoid to invalidate the  
>>>> declaration and
>>>> thus to inhibit ast generation if this is not strictly needed.
>>> Mmm... yeah, even if we don't fix this, we probably don't need to  
>>> mark
>>> the declaration invalid; it's well-formed in any case.
>>> -Eli
>> Hello.
>>
>> Please find attached a small patch that fixes what is, to my  
>> understanding, another occurrence of an unneeded declaration  
>> invalidation (triggered by non-standard use of VM types).
>>
>> The testcase is the following:
>> ====================
>> void p(int a) {
>>  struct {
>>    char x[(int)(char*)2 - 4000];
>>    char y[a];
>>  } f;
>> }
>> [snip]
>>
>> Hence, the error is still reported as it was,
>> but the AST generation is more faithful to the original.
>> The latter property is important for clients such as ours.
>>
>> Cheers,
>> Enea Zaffanella.
>> Index: lib/Sema/SemaDecl.cpp
>> ===================================================================
>> --- lib/Sema/SemaDecl.cpp	(revision 74831)
>> +++ lib/Sema/SemaDecl.cpp	(working copy)
>> @@ -3962,12 +3962,12 @@
>>       Diag(Loc, diag::warn_illegal_constant_array_size);
>>       T = FixedTy;
>>     } else {
>> +      // Do not unnecessarily invalidate declaration
>> +      // (the AST is well-formed anyway).
>>       if (SizeIsNegative)
>>         Diag(Loc, diag::err_typecheck_negative_array_size);
>>       else
>>         Diag(Loc, diag::err_typecheck_field_variable_size);
>> -      T = Context.IntTy;
>> -      InvalidDecl = true;
>>     }
>>   }
> 
> The real problem here is that we're replacing the type of the field  
> with 'int', when there's no reason to do that. However, I feel that it  
> is important to mark this declaration as invalid, because it *is*  
> invalid. So, I've committed the change not to replace the type with  
> 'int', but the declaration is still marked invalid:
> 
> 	http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090720/019240.html

The problem with this choice is that the AST is needlessly mutilated.
Take this example:

void f(int n) {
  struct s {
    int i;
    int a[n];
  };
  struct s a, b;
  a.i = 3;
  a.a[0] = 2;
}

With your changes the last statement of the function is not inserted in
the AST.

Which drawbacks you see to not invalidate the declaration and then to
permit the building of a complete AST nevertheless, once taken for
granted that the code generation phase will never be run?