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: > 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! Oh please wait for others to review, I don't think landing today is reasonable! I am not really a stakeholder for this checker except for that original change. I would advise for you to wait for another more responsible reviewer to accept as well before merging, unless this gets stale for a long time and no one else seems to be interested. 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
