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. https://reviews.llvm.org/D49112 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits