[cfe-dev] Unneeded declarations invalidation

[Enea Zaffanella]

Eli Friedman wrote:
> On Tue, Jul 7, 2009 at 3:46 AM, Enea Zaffanella<zaffanella at cs.unipr.it> wrote:
>> Index: include/clang/Basic/DiagnosticSemaKinds.td
>> ===================================================================
>> --- include/clang/Basic/DiagnosticSemaKinds.td  (revision 74831)
>> +++ include/clang/Basic/DiagnosticSemaKinds.td  (working copy)
>> @@ -1191,6 +1191,8 @@
>>   "arithmetic on pointer to void type">;
>>  def err_typecheck_decl_incomplete_type : Error<
>>   "variable has incomplete type %0">;
>> +def warn_typecheck_decl_incomplete_type : Warning<
>> +  "variable has incomplete type %0">;
> 
> This should be an ExtWarn; it's generally the preferred classification
> for issues which make the translation unit undefined at compile-time.
> Also, it would be nice to explicitly point out the issue, more like
> "tentative definition of variable with internal linkage has incomplete
> type %0" (I actually don't particularly like that wording, but the
> issue is a bit technical, so I'm not sure how to rephrase it.)

The message now reads:

"tentative definition of variable with internal linkage has incomplete 
non-array type %0"

This also tells that an array type would directly lead to an error 
(which is the behavior in gcc).

>> +        // NOTE: code such as the following
>> +        //     static struct s;
>> +        //     struct s { int a; };
>> +        // is accepted by gcc. Hence here we issue a warning instead of
>> +        // an error and we do not invalidate the static declaration.
>> +        (void) RequireCompleteType(IDecl->getLocation(), T,
>> +
>> diag::warn_typecheck_decl_incomplete_type);
> 
> LLVM style generally does not include explicit casts to void.
> 
> 
> It's not ideal that we produce a warning and then an error, but it
> doesn't really matter too much.
> 
> Does this patch react reasonably to multiple tentative definitions?
> It looks like it should, but it's probably worth having a test for it.

Not really sure what you mean by "react reasonably".
If I fed in the following input:

static struct S a;
static struct S a;

then I was getting a total of 6 diagnostics:
  - 2 warnings for the 2 tentative definitions;
  - 1 error for the missing type completion;
  - 3 notes.

I have now applied a workaround that turns off the repeated warning.
The workaround is based on the following observation: if present at all, 
the "static" storage-class specifier should occur in *all* of the 
declarations (otherwise a different error will be obtained).
Hence, it is enough to issue the warning on the first declaration.
This way, I obtain the following:

=======================
goo.c:1:17: warning: tentative definition of variable with internal 
linkage has
       incomplete non-array type 'struct S'
static struct S a;
                 ^
goo.c:1:15: note: forward declaration of 'struct S'
static struct S a;
               ^
goo.c:2:17: error: tentative definition has type 'struct S' that is never
       completed
static struct S a;
                 ^
goo.c:1:15: note: forward declaration of 'struct S'
static struct S a;
               ^
4 diagnostics generated.
=======================


NOTE: if multiple warnings were the desired behavior, then all you need 
to do is to drop the following lines from the patch:

+        // NOTE: to avoid multiple warnings, only check the first 
declaration.
+        if (IDecl->getPreviousDeclaration() == 0)


> 
> Otherwise, it looks fine.
> 
> -Eli
> 
> 

Please find attached the revised patch.


Cheers,
Enea.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: StaticForwardDeclStruct-revised.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090708/f1bdd08a/attachment.ksh>