void test(int *arr) {
int arr_len = sizeof(arr) / sizeof(*arr); // warn, incorrect way to compute number of array elements
}
Enabled under -Wall (same behaviour as GCC)
Paths
| Differential D52949
[Diagnostics] Implement -Wsizeof-pointer-div ClosedPublic Authored by xbolva00 on Oct 5 2018, 2:41 PM.
Details Summary void test(int *arr) { int arr_len = sizeof(arr) / sizeof(*arr); // warn, incorrect way to compute number of array elements } Enabled under -Wall (same behaviour as GCC)
Diff Detail Event Timeline
Comment Actions It looks like you ran clang-format on all of lib/Sema/SemaExpr.cpp and changed many lines that are irrelevant to your patch. Can you undo that, please? Comment Actions Can you add tests with arrays? Comment Actions
Yes :)
What do you suggest? "division produces the incorrect number of array elements; pass the length of array as a function argument"?
Ok, good idea. Comment Actions Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers) uint32_t data[] = {10, 20, 30, 40}; Comment Actions
IMHO, a clang-tidy checker is more suitable than clang static analyzer to indicate to the developers that we should prefer std::size to sizeof(data)/sizeof(*data). What I want to know most is whether we should emit a warning for the code below. I haven't been able to test the patch yet, but intuitively, this patch will emit a warning. int foo(int *p) { int d = sizeof(p) / sizeof(char); // Should we emit a warning here? return d; } GCC keep silent about this case, and I agree with it, see https://gcc.godbolt.org/z/ZToFqq. What do you think about this case? Comment Actions Any ideas for better warning message? Except for the warning text, I see this patch as ready. Comment Actions Sorry for the long delay for this patch! The implementation is fine for me. However, I'm the newbie on clang diagnostics and have no further insight into this checker. @aaron.ballman may have more valuable insights into this checker.
xbolva00 marked 5 inline comments as done. Comment Actions
We match the GCC behaviour here, except the case: But I think it is ok now.
Comment Actions LGTM with a minor formatting nit.
This revision is now accepted and ready to land.Nov 1 2018, 7:44 AM Closed by commit rL345847: [Diagnostics] Implement -Wsizeof-pointer-div (authored by xbolva00). · Explain WhyNov 1 2018, 9:29 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 168546 include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.c
|
Do you really need the explicit diagnostic group? Given that there's only one diagnostic here, I would lower this into the diag (see below).