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

[Ed Schouten]

Hi Eli,

Sorry for the very late response, but the last months were a bit busy
for me (graduating, migrating).

2011/12/15 Eli Friedman <eli.friedman at gmail.com>:
> I'm pretty sure we call CheckCompleteVariableDeclaration for both
> those declarations; I could be mistaken, though.

I just checked and it seems CheckCompleteVariableDeclaration is not
called for those declarations. Maybe it's only called for variable
declarations with an initializer?

>> I'm not a very good compiler hacker so I could use some help here. I
>> suspect you mean something like this?
>>
>>  if (var->isThisDeclarationADefinition() &&
>>      var->getLinkage() == ExternalLinkage &&
>>      var->getPreviousDeclaration() == NULL)
>>    Diag(var->getLocation(),
>>      diag::warn_missing_variable_declarations) << var;
>
> Yes.

It seems getPreviousDeclaration() has been removed, so I've fallen
back to simply checking against `Previous.empty()'. If anyone knows of
a more elegant solution, please let me know.

The latest version of the patch can be found here:

http://80386.nl/pub/wmissing-variable-declarations.txt

As this thread went a bit stale over the last couple of months, let me
rehash what this patch is about.

The -Wmissing-variable-declarations flag adds a warning for the following:

int i; // Global variable declaration that has no prior extern
declaration and is not static: warn
static int j; // Don't warn
extern int k;
int k; // Don't warn

One can say it's similar to -Wmissing-prototypes, but then for
variables. In my opinion it is good to have such a warning flag for
the following reasons:

- It makes it easier to find dead code, namely unused global
variables. If you get one of these warnings and decide to mark the
variable static, you will immediately get a compiler warning about an
unused global variable.
- There are cases in which marking as many things static has a
positive impact on the number of optimisations that can be performed.
For example, consider the following piece of code:

int nresults;

void findresults(void)
{
    nresults = 0;
    somefunction();
    while (nresults == 0) {
        // find results
    }
}

Because `nresults' is not static, the compiler cannot rely on
`somefunction()' to leave `nresults' alone. By marking it static (and
under the assumption that a pointer to `nresults' never leaves the
compilation unit), the compiler is free to optimize the while-loop to
a do-while-loop.

- People often forget to use const-keywords properly:

const char *myblacklist[] = { "apples", "oranges", NULL };

When this array can be marked static, Clang is smart enough to place
the array in a read-only segment when it detects that the contents of
the array are not modified This may potentially be a good thing from a
security point of view.

- It may in some cases find real bugs in your code.

C (maybe just ELF?) has this `awesome' feature that it merges global
variables if they have the same size (and are not initialized multiple
times). If one C file contains `long x;' and the other one contains
`double x;', all sorts of funny things might happen. This warning flag
could spot issues like this.

I used this patch set last year on the FreeBSD source tree and I
observed that simply sprinkling some more static-keywords throughout
the code of code had some positive impact. Some highlights:

- stty(1): though the binary is only 22 KB in size, 3.5 KB of data in
the binary section became read-only.
- The size of the tools in /bin decreased by approximately 1%.

Thanks for your patience,
-- 
Ed Schouten <ed at 80386.nl>