[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63423#1550713 <https://reviews.llvm.org/D63423#1550713>, @jfb wrote:

> In D63423#1550705 <https://reviews.llvm.org/D63423#1550705>, @aaron.ballman wrote:
>
> > In D63423#1550697 <https://reviews.llvm.org/D63423#1550697>, @jfb wrote:
> >
> > > What I meant with macros was that I don't think we should warn on:
> > >
> > >   #define LEGIT(a, b) ({ work work work; a ^ b; work work work; })
> > >  
> > >   LEGIT(10, 5);
> > >
> > >
> > > If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote `CONSTANT ^ CONSTANT` and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.
> >
> >
> > So you would not want to see this code diagnosed?
> >
> >   #define POW(x, y) (x ^ y)
> >  
> >   void f() {
> >     int two_to_the_derp = POW(2, 16);
> >   }
> >
> >
> > I'm not certain I understand the rationale for why we would not want to diagnose such a construct. Do we have reason to expect a lot of false positives?
>
>
> If you wrote `CONSTANT_2_OR_10 ^ CONSTANT` I'm pretty sure you messed up. Macros are opaque, and I'm not sure you clearly messed up anymore. Let's take the clear win, see what falls out, and only expand our reach if we have reason to do so. Without trying on huge codebases I don't know whether we'll get a lot of false positive. I'd rather be conservative and not cause headache.


IIRC, we do not usually go out of our way to stop diagnosing code within macro expansions unless there's a reason to do so. Given that we know the code is likely wrong (because we're only checking constants whether inside or outside of a macro expansion) and that we've seen the incorrect code happen inside of macros in the wild, I suspect that being conservative is not much of a win. That said, I definitely understand the desire to not issue burdensome amounts of false positives. If we see false positives coming from macros, we should address them (perhaps even by not diagnosing within a macro). Perhaps the author can run the check over a large corpus of code to see whether fps come up in practice? (The presence of fps will suggest to not warn in macros, but the absence of fps won't tell us too much.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63423/new/

https://reviews.llvm.org/D63423