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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
