[PATCH] Warn on mismatching types to sizeof for memset and friends where length is of the form sizeof(Type) * factor.

[Nico Weber]

The code looks reasonable to me, but others on cfe-commits know Sema better than I.

clang generally has a high bar for adding warnings. To evaluate a warning, we usually run it on some large code base and measure true and false positives and then compare the severity of the true positives with the noise from the false positives. Do you have any data on what the warning warns on, how serious the bugs it finds are, and if there are any common patterns this warns on but shouldn't?


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:435
@@ -432,1 +434,3 @@
+  "%select{destination|source}2; expected %3">,
+  InGroup<SizeofPointerMemaccess>;
 def warn_strlcpycat_wrong_size : Warning<
----------------
I wonder if this should have its own flag instead of going into the existing group – else this might be annoying for people who have the current version of this warning enabled. I'm not sure though. (Do others have opinions?)

================
Comment at: lib/Sema/SemaChecking.cpp:4741
@@ +4740,3 @@
+
+static bool typesAreCompatibleIgnoreQualifiers(ASTContext &Context,
+                                               QualType &LHSType,
----------------
It feels like we should have a function for this already…I couldn't find one either though.

================
Comment at: lib/Sema/SemaChecking.cpp:4758
@@ +4757,3 @@
+                                            QualType &EltType) {
+    QualType LHSType =
+        Context.getCanonicalType(SizeOfArgFactorTy).getUnqualifiedType();
----------------
clang uses a 2 space indent. Please run clang-format on your patch.

================
Comment at: lib/Sema/SemaChecking.cpp:4916
@@ +4915,3 @@
+                                << EltType << Dest->getSourceRange()
+                                << LenExpr->getSourceRange());
+    }
----------------
Should this also emit a "cast to (void*) to suppress warning" fixit like other warnings here do?

If the dest is an array with a known size, and the literal is the size of the array, should we suggest sizeof(variablename) instead?

================
Comment at: test/SemaCXX/warn-memset-bad-sizeof.cpp:4
@@ +3,3 @@
+
+#include <stddef.h>
+
----------------
Tests should generally be standalone and not include any headers. What do you need this for? size_t? If so, do `typedef __SIZE_TYPE__ size_t;` instead.

http://reviews.llvm.org/D8881

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/