rjmccall added a comment. In D63856#1561132 <https://reviews.llvm.org/D63856#1561132>, @erik.pilkington wrote:
> In D63856#1561112 <https://reviews.llvm.org/D63856#1561112>, @rjmccall wrote: > > > In D63856#1560213 <https://reviews.llvm.org/D63856#1560213>, > > @erik.pilkington wrote: > > > > > In D63856#1560180 <https://reviews.llvm.org/D63856#1560180>, @rjmccall > > > wrote: > > > > > > > This only applies to relational operators, right? I'm a little > > > > uncomfortable with calling this "tautological" since it's not like it's > > > > *undefined behavior* to have `(BOOL) 2`, it's just *unwise*. But as > > > > long as we aren't warning about reasonable idioms that are intended to > > > > handle unfortunate situations — like other code that might have left a > > > > non-`{0,1}` value in the `BOOL` — I think this is fine. > > > > > > > > > I think the party line is that it is undefined behaviour (in some sense), > > > since UBSan will happily crash if you try to load a non-boolean value > > > from a BOOL. > > > > > > What? Since when? > > > https://reviews.llvm.org/D27607 Interesting; I'm not sure I find that convincing. Probably the least convincing part is where it links to its own behavioral documentation as justification for doing what it does. But okay, I guess we do this. >>> It is a bit unfortunate that "defensive" code will start warning here >>> though :/. Maybe we can try to detect and permit something like `B < NO || >>> B > YES`, or emit a note with some canonical way of checking for >>> non-boolean BOOLs. Even if we end up having to disable it default, I think >>> its still a good diagnostic to have. A warning on stores to BOOL would >>> probably be a lot higher value, though. >> >> I'm not sure this is a problem because I'm not sure there's any reason to >> write defensive code besides `B != NO` or `B == NO`. It's potentially >> problematic if someone writes `B == YES`, though. > > I was thinking about something like the following, which probably isn't worth > warning on. Another way of handling it would be only emitting the diagnostic > if `!InRange`. Not exactly sure how common that pattern actually is though. > > void myAPI(BOOL DoAThing) { > if (DoAThing > YES || DoAThing < NO) DoAThing = YES; > // ... > } I don't think it's a problem to warn about this; this is just a silly way of writing `if (DoAThing != NO) { DoAThing = YES; }`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63856/new/ https://reviews.llvm.org/D63856 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
