A patch to configure the "pointer to aggregate" warning in bugprone-sizeof-expression

clang-tidy has the check bugprone-sizeof-expression to warn about possibly erroneous uses of sizeof. One thing it warngs about is applying sizeof to a pointer-to-aggregate. However, some projects get many false positives for this subcategory of warning, notably Chromium. I’d like to create a patch with a new option WarnOnSizeOfPointerToAggregate so this could be enabled or disabled at will. Would that be welcome?

I wonder if this is a problem for you now due to recent changes.

There is one buggy matcher in that check, the one which handles sizeof(record_type) (as opposed to sizeof(expr)).

You can see more details about it in the reviews, but ⚙ D112374 [clang] Implement ElaboratedType sugaring for types written bare fixed the original bug in the matcher, but as a consequence expanded enormously the amount of positives.

There were complains about the new potential false positives, and we decided, in ⚙ D131926 [clang-tidy] Fix for bugprone-sizeof-expression PR57167, to basically just knock this matcher off until we come up with a good alternative for it.

Is this high amount of false positives still an issue in trunk? That would be strange because it should be matching even less cases than before D112374.

Yes, it appears we still get many false positives with a new clang-tidy.

I used llvm-project commit ddc939fe15e420e2d3d9d156a8cfa8ce790573ab and built clang-tidy from that.

Previously I got 205 triggers of the “pointer to aggregate” warning. Now I only get 89, but a random sample of 15 of them are all false positives.

It does seem strange that a checker named bugprone-sizeof-expression has a matcher for sizeof(type).

Maybe it would make more sense to remove that matcher, effectively a partial revert of ⚙ D61260 [clang-tidy] Extend bugprone-sizeof-expression to check sizeof(pointers to structures), since it does not seem to be working well and is barely tested.

Then if we decide to add this kind of check again, we can come up with sensible rules for it and name the new checker something like bugprone-sizeof-type.

Can you by the way give us some examples of those false positives?

Sure, here are some examples:

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/libxml/src/parser.c#1814

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/blink/renderer/platform/wtf/thread_specific.h#126

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/libxslt/src/libxslt/transform.c#2231

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/abseil-cpp/absl/container/internal/common.h#114

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/libxml/src/xmlreader.c#547

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/abseil-cpp/absl/container/internal/raw_hash_set.h#1996

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/abseil-cpp/absl/types/internal/optional.h#121

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/gpu/command_buffer/service/logger.cc#26

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/components/visitedlink/browser/visitedlink_writer.cc#705

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/components/visitedlink/browser/visitedlink_writer.cc#575

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/abseil-cpp/absl/container/internal/raw_hash_set.h#2027

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/leveldatabase/env_chromium.cc#198

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/libxslt/src/libxslt/transform.c#320

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/libevent/min_heap.h#114

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/blink/renderer/platform/wtf/hash_table.h#1792

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/abseil-cpp/absl/container/internal/raw_hash_set.h#1999

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/blink/renderer/platform/wtf/hash_traits.h#52

Ah okay, most of these do fall under the sizeof(expr) category and were not affected by the recent changes.

Sure, I think making that configurable is a good idea, and I have seen some support for that direction in reading comments on those reviews I linked.