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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131926