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 ---------------- 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. 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
