[PATCH] D52949: [Diagnostics] Implement -Wsizeof-pointer-div

[Aaron Ballman via Phabricator via cfe-commits]

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