On 04/22/17 01:50, Martin Sebor wrote:
> On 04/20/2017 02:35 PM, Bernd Edlinger wrote:
>> Hi!
>>
>>
>> This implements a new -Wall enabled warning for a rather common, but
>> completely wrong way to compute an array size by dividing the
>> sizeof(pointer) / sizeof(pointer[0]) or sizeof(*pointer).
>>
>> It is often hard to find this kind of error by simple code inspection
>> in real code, because using sizeof in this way is a quite common idiom
>> to get the array size of an array variable.  And furthermore this
>> expression may be used in macros, which makes it even more important to
>> have this warning.
>>
>> There is a similar warning -Wsizeof-pointer-memaccess which helped in
>> implementing the infrastructure for the new warning in the C FE.
>>
>> However I noticed that the -Wsizeof-pointer-memaccess warning was
>> missing in C, when the sizeof is used inside parentheses, which is
>> different from C++, so I fixed that too.
>>
>> Of course, I added some test cases for that as well.
>>
>> To illustrate the usefulness of this warning, it revealed quite a few
>> places where bogus sizeof divisions were used in our testsuite.
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>
> That seems like a useful warning.  Just a few comments.
>
> First, -Wsizeof-array-argument already diagnoses a subset of
> the same problems.  For example, with the patch applied, GCC
> issues the two warnings below for following test case.  One
> should be sufficient.
>
>   $ cat y.c && gcc -S -Wall y.c
>   int f (int a[])
>   {
>     return sizeof a / sizeof *a;
>   }
>   y.c: In function ‘f’:
>   y.c:3:17: warning: ‘sizeof’ on array function parameter ‘a’ will
> return size of ‘int *’ [-Wsizeof-array-argument]
>      return sizeof a / sizeof *a;
>                    ^
>   y.c:1:12: note: declared here
>    int f (int a[])
>               ^
>   y.c:3:19: warning: dividing the pointer size by the element size
> [-Wsizeof-pointer-div]
>      return sizeof a / sizeof *a;
>                      ^
>
> Second, I would suggest mentioning the actual types of the operands
> rather than referring to "pointer size" and "element size."  Maybe
> something like:
>
>   division 'sizeof (int*) / sizeof (int)' does not compute the number of
> array elements
>
> I suggest avoiding "element size" because the pointed-to argument
> need not be an array.  Mentioning the types should help users better
> understand the problem (especially in C++ where types are often
> obscured by layers of templates).  It might also be a nice touch
> to add a note pointing to the declaration of the first sizeof
> operand (if it's an object).
>

Yes, many thanks for your suggestions.

Do the new warning and info messages look right?


Is it OK for trunk after bootstrap / reg-testing?


Thanks
Bernd.