[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D44231#1032782, @pfultz2 wrote:

> > Again, that only works for C++ and not C.
>
> Typedef has always worked in C.


This is true.

I think what I'm struggling to say is: I don't think this is a common pattern in either C or C++. It's also unnecessary because you can avoid repeating the type by just using `sizeof` on the function call result.

>> Did it report any true positives that would need correcting?
> 
> Not for LLVM, but it has in other projects like I mentioned.
> 
>> Can you check some other large repos (both C++ and C), such as: Qt, cocos2d, rethinkdb, redis, and php-src?
> 
> None of those projects use cmake, so it doesn't look easy to run clang-tidy on them. Do you have a script that will run clang-tidy on these repos?

I don't have a script for it. I've used "bear" with at least some of those projects because they use makefiles rather than cmake (https://github.com/rizsotto/Bear). I'm not tied to those projects specifically, either, so if you have a different corpus you'd prefer to use, that's fine. The important bit is testing it across a considerable amount of C code and C++ code to see whether this diagnostic is too chatty or not.

> Actually, PVS-Studio has a more general check for `sizeof(expr)`:
> 
> https://www.viva64.com/en/examples/v568/
> 
> It shows an example of FCEUX doing `sizeof(buf - 1)`, Asterisk doing `sizeof(data[0] * 2)` and ReactOS doing `sizeof(UnknownError [0] - 20)`. I think it might be a good idea to expand this from just a function call to any integer expression.

That won't catch many (most?) of the issues demonstrated by PVS-Studio; the rule their check follows are to warn on side-effecting operations (which Clang already does with -Wunevaluated-expression) and arithmetic expressions in sizeof. The latter might be a reasonable extension to the check -- I have less concerns about the false positive rate with that than I do with the function call case. Another possible extension to consider is `sizeof(sizeof(foo))`, which it seems PVS-Studio will diagnose as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44231