[PATCH] D13643: [Sema] Warn on ternary comparison

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In http://reviews.llvm.org/D13643#266649, @mgrabovsky wrote:

> In http://reviews.llvm.org/D13643#265976, @aaron.ballman wrote:
>
> > Should there be an exception to this diagnostic for code involving Boolean values? e.g.,
> >
> > void f(bool a, bool b, bool c) {
> >
> >   if (a == b == c)
> >     ;
> >
> > }
> >
> > At the very least, it seems like this should also follow GCC's behavior and suggest parenthesis directly.
>
>
> Interesting. This might be more complex than I had predicted. I'll try to dig into GCC's code and see what they do exactly. I can't guarantee returning sane, though.
>
> Some thoughts: Even if `a, b, c` are Booleans, what did the user mean by `a == b == c`? Do they realise that the compiler interprets it as `(a == b) == c`? What about 'type mismatches', i.e. if `a, b` are `int`s, while `b` is `bool` – clearly the user must be aware of the interpretation above here. What if `a, b, c` are all `int`s – C90 doesn't have Booleans, so it might be intended as either.


I would spend some time digging into how GCC handles those cases, and use that as a baseline that we can then improve upon. I like the fact that GCC basically says "use parens to clarify your intent", as that solves all of the cases regarding equality and inequality.

I would imagine type == type == bool should behave the same as bool == bool == bool; it can be valid code that just needs parens to clarify intent. As for C90 behavior, I'm not too worried about what we do there.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5865
@@ -5864,1 +5864,3 @@
 
+def warn_ternary_comparison : Warning<"ternary comparisons do not work "
+  "as expected">,
----------------
mgrabovsky wrote:
> aaron.ballman wrote:
> > This diagnostic somewhat implies that ?: does not work as expected. I prefer GCC's wording for this:
> > 
> > "comparisons like 'X<=Y<=Z' do not have their mathematical meaning"
> > 
> > However, I would love it if we could do one step better and use the same operators the user wrote, if reasonable. ;-)
> > This diagnostic somewhat implies that ?: does not work as expected.
> 
> ?: is a ternary operator but it's not a comparison.
> 
> > I prefer GCC's wording for this
> 
> I wasn't sure if we wanted to be copycats, but OK.
> 
> > would love it if we could do one step better and use the same operators the user wrote
> 
> Yes, that would be nice.
> I wasn't sure if we wanted to be copycats, but OK.

If we can improve on their wording, I am all for it. I just find their wording more clear than the proposed wording.


http://reviews.llvm.org/D13643