This revision was automatically updated to reflect the committed changes.
Closed by commit rL345847: [Diagnostics] Implement -Wsizeof-pointer-div
(authored by xbolva00, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with a minor formatting nit.
Comment at: lib/Sema/SemaExpr.cpp:8729
+static void DiagnoseDivisionSizeofPointer(Sema , Expr *LHS, Expr *RHS,
+
xbolva00 added a comment.
- Addressed review comments.
We match the GCC behaviour here, except the case:
int b1 = sizeof(int *) / sizeof(int);
But I think it is ok now.
Comment at: lib/Sema/SemaExpr.cpp:8729
+static void DiagnoseDivisionSizeofPointer(Sema , Expr *LHS, Expr
xbolva00 updated this revision to Diff 172117.
xbolva00 marked 5 inline comments as done.
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.cpp
Index: test/Sema/div-sizeof-ptr.cpp
aaron.ballman added inline comments.
Comment at: include/clang/Basic/DiagnosticGroups.td:147
def : DiagGroup<"div-by-zero", [DivZero]>;
+def DivSizeofPtr : DiagGroup<"sizeof-pointer-div">;
Do you really need the explicit diagnostic group? Given that there's
MTC added a comment.
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.
Comment at:
xbolva00 updated this revision to Diff 171920.
xbolva00 added a reviewer: MTC.
xbolva00 added a comment.
- New warning text, added another test line
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
xbolva00 added a comment.
Any ideas for better warning message? Except for the warning text, I see this
patch as ready.
https://reviews.llvm.org/D52949
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
xbolva00 added a comment.
Ping @MTC or @rsmith
https://reviews.llvm.org/D52949
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 updated this revision to Diff 170696.
xbolva00 added a comment.
- Added MTC'c case to test file
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.c
Index:
xbolva00 added a comment.
No warning for your case.
https://reviews.llvm.org/D52949
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MTC added a comment.
In https://reviews.llvm.org/D52949#1268640, @xbolva00 wrote:
> 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};
> len = sizeof(data)/sizeof(*data); // "warn" on
xbolva00 added a comment.
Ping
https://reviews.llvm.org/D52949
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 added a comment.
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};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend
std::size? I dont agree with such
xbolva00 added a comment.
In https://reviews.llvm.org/D52949#1259947, @jfb wrote:
> Can you add tests with arrays?
Yes :)
> The error message doesn't really say what to do instead. What's wrong? What's
> correct instead?
What do you suggest? "division produces the incorrect number of array
jfb added a comment.
Can you add tests with arrays?
The error message doesn't really say what to do instead. What's wrong? What's
correct instead?
In C++17 and later we should suggest using `std::size` for some cases instead.
https://reviews.llvm.org/D52949
xbolva00 added a comment.
ping
https://reviews.llvm.org/D52949
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 updated this revision to Diff 168569.
xbolva00 added a comment.
- added DefaultIgnore
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.c
Index:
xbolva00 updated this revision to Diff 168564.
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.c
Index: test/Sema/div-sizeof-ptr.c
xbolva00 updated this revision to Diff 168562.
xbolva00 added a comment.
- fixed formatting noise
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.c
Index:
thakis added a comment.
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?
https://reviews.llvm.org/D52949
___
cfe-commits mailing list
xbolva00 added inline comments.
Comment at: test/Sema/div-sizeof-ptr.c:9
+
+int e = sizeof(int *) / sizeof(int);
+int f = sizeof(p) / sizeof(p);
GCC warns also in this case, but it is weird...
https://reviews.llvm.org/D52949
xbolva00 updated this revision to Diff 168539.
https://reviews.llvm.org/D52949
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/div-sizeof-ptr.c
Index: test/Sema/div-sizeof-ptr.c
xbolva00 created this revision.
Herald added a subscriber: cfe-commits.
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)
Repository:
rC Clang
24 matches
Mail list logo
