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 only one
diagnostic here, I would lower this into the diag (see below).
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3299
+ "'%0' will return the size of the pointer, not the array itself">,
+ InGroup<DivSizeofPtr>, DefaultIgnore;
+
----------------
`InGroup<DiagGroup<"sizeof-pointer-div">>`
================
Comment at: lib/Sema/SemaExpr.cpp:8729
+static void DiagnoseDivisionSizeofPointer(Sema &S, Expr *LHS, Expr *RHS,
+ SourceLocation Loc) {
----------------
`const Expr *`
================
Comment at: lib/Sema/SemaExpr.cpp:8731-8733
+ UnaryExprOrTypeTraitExpr *LUE =
+ dyn_cast_or_null<UnaryExprOrTypeTraitExpr>(LHS);
+ UnaryExprOrTypeTraitExpr *RUE =
----------------
Can use `const auto *` here as the type is spelled out in the initialization.
Under what circumstances would you expect to get a null `LHS` or `RHS`? I
suspect this should be using `dyn_cast` instead.
================
Comment at: lib/Sema/SemaExpr.cpp:8746-8750
+ if (RUE->isArgumentType()) {
+ RHSTy = RUE->getArgumentType();
+ } else {
+ RHSTy = RUE->getArgumentExpr()->IgnoreParens()->getType();
+ }
----------------
Elide braces.
================
Comment at: test/Sema/div-sizeof-ptr.c:9
+
+ int e = sizeof(int *) / sizeof(int);
+ int f = sizeof(p) / sizeof(p);
----------------
xbolva00 wrote:
> GCC warns also in this case, but it is weird...
I think it's reasonable to not diagnose here, but I don't have strong feelings.
================
Comment at: test/Sema/div-sizeof-ptr.c:1
+// RUN: %clang_cc1 %s -verify -Wsizeof-pointer-div -fsyntax-only
+
----------------
This is missing tests for the positive behavior -- can you add some tests
involving arrays rather than pointers? Here are some other fun test cases as
well:
```
int a[10][12];
sizeof(a) / sizeof(*a); // Should this warn?
```
```
template <typename Ty, int N>
int f(Ty (&Array)[N]) {
return sizeof(Array) / sizeof(Ty); // Shouldn't warn
}
```
https://reviews.llvm.org/D52949
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
