aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c:42 + sum += sizeof(__typeof(&TS)); + sum += sizeof(STRKWD MyStruct*); + sum += sizeof(__typeof(STRKWD MyStruct*)); ---------------- Based on the documentation, I would expect this one to be diagnosed. ================ 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: > > 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.) 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
