[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args

Erik Pilkington via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 10 10:07:31 PDT 2018

erik.pilkington added inline comments.

Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:662
+def note_suspicious_sizeof_memset_silence : Note<
+  "%select{parenthesize the third argument|cast the second argument to 'int'}0 to silence">;
Quuxplusone wrote:
> If it were my codebase, I'd rather see a cast to `(unsigned char)` than a cast to `(int)`. (The second argument to memset is supposed to be a single byte.) Why did you pick `(int)` specifically?
I chose `int` because that's the actual type of the second parameter to `memset`, it just gets casted down to `unsigned char` internally. FWIW, either type will suppress the warning. I'm fine with recommending `unsigned char` if you have a strong preference for it.

Comment at: clang/lib/Sema/SemaChecking.cpp:7962
+  if (auto *BO = dyn_cast<BinaryOperator>(SizeofExpr)) {
+    if (BO->getOpcode() != BO_Mul)
+      return false;
Quuxplusone wrote:
> Honestly, if the second argument to `memset` involves `sizeof` *at all*, it's probably a bug, isn't it?
>     memset(&buf, sizeof foo + 10, 0xff);
>     memset(&buf, sizeof foo * sizeof bar, 0xff);
> Should Clang really go out of its way to silence the warning in these cases?
Sure, that seems reasonable. The new patch makes this check less conservative.


More information about the cfe-commits mailing list