[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#1031503, @pfultz2 wrote:

> > Can you point to some real world code that would benefit from this check?
>
> Yes, here:
>
> https://github.com/ROCmSoftwarePlatform/MIOpen/blob/master/src/convolution.cpp#L184
>
> It is erroneously calling `sizeof(yDesc.GetType())`. The `GetType` function returns an enum that represents the float type. In this case it wants to compute the size of the float type not the size of the enum that represents the float type. There's actually many instances of this error in the codebase.


Thank you for the example.

>> I don't see this as being a common issue with code and I am concerned about false positives.
> 
> I have seen it as a common issue. It only runs for a function that returns an integer type. Another possibility is to not warn for function calls that are dependent on a template parameter.
> 
> Of course, there is still the option to disable it.
> 
>> For instance, it's reasonable to write sizeof(func_call()) in a context where you don't want to repeat the type name in multiple places.
> 
> Generally, a typedef is used to avoid repeating the type.

The typedef doesn't avoid the repeating type.

  uint16_t foo();
  ...
  size_t s1 = sizeof(foo()); // No repeated type
  size_t s2 = sizeof(uint16_t); // Repeated type

If the return type of `foo()` is changed, then the use for `s1` will be automatically updated while the use for `s2` will not. Your example of how to work around it using `decltype` isn't obvious and won't work for C users.

>> I've seen this construct used in LLVM's code base (see Prologue::getLength() in DWARFDebugLine.h for one such instance).
> 
> This is the first time I have seen this used legitimately outside of template metaprogramming. Alternatively, you could also write `sizeof(decltype(expr))`.

I think we need some data measurements over several large (>500k LoC) C and C++ code bases to see how frequently this check yields false positives. I have a hard time imagining that the true positives will outweigh the false positives, but without concrete data, it's just guesswork.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44231