[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

Matheus Izvekov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 16 08:24:18 PDT 2022


mizvekov added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
----------------
chrish_ericsson_atx wrote:
> njames93 wrote:
> > mizvekov wrote:
> > > Is this change really desirable, or should we put a FIXME here?
> > Not warning on these cases seems like a pretty big red flag, especially the `MyStruct *`.
> Thank you for your comment!  I'm not sure I fully agree that this is a red flag.  I'm inclined to think that whether or not there should be a warning on `MyStruct *` or `PMyStruct` is a pretty subjective.  These are both very common idioms, and are meaningful.  I do appreciate the perspective that `MyStruct *` is just one character different from `MyStruct`, and as such, it might be a typo to ask for sizeof the pointer, when you really wanted sizeof the struct.  But the counter-argument (accidentally asking for sizeof the struct when you really wanted the pointer size) is just as valid-- and the checker obviously cannot warn in that case.   
> 
> I am perfectly fine with kicking the can down the road a bit by replacing the discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as @mizvekov suggested.  I expect that when someone circles back to really deeply reconsider the semantics of this checker, there will be a number of changes (other existing warnings dropped, warnings for new and missed cases added, much better sync between C and C++, as well as intentional consideration of things like __typeof__ (in it's various forms) and decltype, rather than the haphazard coarse-grain matching that seems to be going on now.
I agree with this patch only in so far that:

* This is effectively a partial revert of the changes made in https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
* The checker was pretty much buggy to begin with.
* That change significantly increased the amount of patterns we would accept, but at the same time the existing tests did not pick that up and this was not carefully considered.
* It seems unreasonable that there is effectively no way to shut this warning up per site, except by a NOLINT directive.

If the amount of false positives is so high now that this check is unusable, then this is just a question of reverting to a less broken state temporarily.

But otherwise we can't leave it in the reverted state either. Without that change to use the canonical type or something similar, there is no reason to suppose any of these test cases would work at all as clang evolves and we improve the quality of implementation wrt type sugar retention.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926



More information about the cfe-commits mailing list