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