aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from removing some comments now that we figured out what's going on. Please hold off on landing for a day or two in case @njames93 has other opinions though.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:235-236 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate - sum += sizeof(MyStruct*); - // 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 + sum += sizeof(MyStruct*); // FIXME: Warning here prior to 15f3cd6bfc6, consider whether to add it back. + sum += sizeof(PMyStruct); // FIXME: Warning here prior to 15f3cd6bfc6, consider whether to add it back. sum += sizeof(PS); ---------------- ================ 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: > aaron.ballman wrote: > > mizvekov wrote: > > > 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. > > > 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 agree with @njames93 that this is a red flag. That check behavior is > > explicitly documented: > > https://releases.llvm.org/13.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-sizeof-expression.html#suspicious-usage-of-sizeof-a > > so this change is introducing a different kind of regression. > > > > (However, there's no option for disabling that specific situation and I > > think there probably should be one, eventually.) > This revert is almost entirely knocking out this `PointerToStructType` > matcher, which matches `sizeof` of any record which is not written as an > address-of operator expression. > > I think with this revert and after the elaboratedType patch changes, the only > case that should still trigger it should be a type substitution for a > template argument written as a pointer to Record, because that should > canonicalize the pointee without adding any other sugar on it. > > It won't match a `pointer-to-sugar-to-record` anymore at all, as it happened > before the ElaborateType patch, but before that patch, there was an > additional case that would match: A pointer to a struct written without any > elaboration, because we would not put any sugar on top of the RecordType. > > The documentation does mention the `Suspicious usage of ‘sizeof(A*)’` case, > but it only gives as an example the `sizeof-address-of-expression` case, even > though that is handled separately in the implementation here and not affected. > > So I guess that might explain the contention here about the seriousness of > this regression. > The documentation does mention the Suspicious usage of ‘sizeof(A*)’ case, but > it only gives as an example the sizeof-address-of-expression case, even > though that is handled separately in the implementation here and not affected. > > So I guess that might explain the contention here about the seriousness of > this regression. Yeah, those docs only show the expression case and not the type case. That's an interesting point! It also says `These cases may occur because of explicit cast or implicit conversion.` which sure implies that this is an expression check rather than a type check. I dug through the history to find the original review for this feature to see if this was discussed. That review is: https://reviews.llvm.org/D19014 I didn't spot any indication this was meant to diagnose `sizeof(some_type)`. So I guess I'm wrong on this being a red flag! 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
