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