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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7984
+  if (isa<IntegerLiteral>(ThirdArg) &&
+      cast<IntegerLiteral>(ThirdArg)->getValue() == 0) {
+    WarningKind = 0;
----------------
Quuxplusone wrote:
> > Suppress the diagnostic in cases like `memset(ptr, 0xff, PADDING)`, when `PADDING` is a macro defined to be `0`.
> 
> Would it suffice to treat a literal `0xFF` in the exact same way you treat a literal `0`? That is,
> ```
> if (SecondArg is integer literal 0 or 255 || ThirdArg involves sizeof) {
>     do nothing
> } else if (ThirdArg is integer literal 0 or 255 || SecondArg involves sizeof) {
>     diagnose it
> }
> ```
> You should add a test case proving that `memset(x, 0, 0)` doesn't get diagnosed (assuming the two `0`s come from two different macro expansions, for example).
> My sketch above would fail to diagnose `memset(x, 0, 255)`. I don't know if this is a feature or a bug. :)  You probably ought to have a test case for that, either way, just to demonstrate the expected behavior.
I definitely don't think that ThirdArg being 255 should lead to a diagnostic, regardless of what SecondArg is. I'm fine with omitting a diagnostic in `memset(ptr, 255, 0)`, but if the user explicitly spelled out `0` then I would probably prefer to diagnose. FWIW, for the case I found where we emitted a false-positive the second argument was 0xd9 or something.


https://reviews.llvm.org/D49112