[cfe-commits] [Patch] Add -Wmissing-variable-declarations

[Eli Friedman]

On Thu, Oct 18, 2012 at 2:57 AM, Ed Schouten <ed at 80386.nl> wrote:
> Hi Eli,
>
> 2012/8/1 Eli Friedman <eli.friedman at gmail.com>:
>> On Tue, Jul 24, 2012 at 7:06 AM, Ed Schouten <ed at 80386.nl> wrote:
>>> I've updated the patch. As usual, it can be downloaded from:
>>>
>>> http://80386.nl/pub/wmissing-variable-declarations.txt
>>
>> @@ -6839,8 +6839,10 @@
>>        }
>>
>>        // Record the tentative definition; we're done.
>> -      if (!Var->isInvalidDecl())
>> +      if (!Var->isInvalidDecl()) {
>>          TentativeDefinitions.push_back(Var);
>> +        CheckCompleteVariableDeclaration(Var);
>> +      }
>>        return;
>>      }
>>
>> Calling CheckCompleteVariableDeclaration here doesn't make sense; the
>> variable isn't complete at this point.  If we're going to call it, it
>> should be from Sema::ActOnEndOfTranslationUnit.
>>
>> Otherwise, looks fine.
>
> Great! Care to take another look?
>
> http://80386.nl/pub/wmissing-variable-declarations.txt
>
> (Be sure to refresh)

+  if (var->isThisDeclarationADefinition() &&
+      (var->getLinkage() == ExternalLinkage ||
+      var->getLinkage() == UniqueExternalLinkage)) {

We don't want to warn for code like the following, which is what
UniqueExternalLinkage is used for:

namespace {
  int x;
}

This is roughly equivalent to "static int x;".  (Please also add a
testcase for this.)

Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp	(Revision 166173)
+++ lib/Sema/Sema.cpp	(Arbeitskopie)
@@ -618,6 +618,7 @@
     // return value is false.
     if (VD == 0 || VD->isInvalidDecl() || !Seen.insert(VD))
       continue;
+    CheckCompleteVariableDeclaration(VD);

     if (const IncompleteArrayType *ArrayT
         = Context.getAsIncompleteArrayType(VD->getType())) {

I think it would make sense to move the call to
CheckCompleteVariableDeclaration below the check that the type is
complete.

+    if (prev == NULL)

Prevailing style is to not explicitly compare against NULL (so "if (prev)".)

-Eli