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 ---------------- mizvekov wrote: > 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. @mizvekov, I agree with your reasoning for saying "we can't leave it in the reverted state either". But I'm not sure how to ensure that this gets the needed attention. Should we just file a separate PR on github to track the needed refactoring? I do not believe I'll have the bandwidth to look at this in the next few months. In the meantime, assuming the bots are happy with patchset 2, I'll land this as-is later today. Thank you very much for your feedback! 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
