No warning on shifted boolean expression

[arsenm]

Bugzilla Link	27960
Version	unspecified
OS	All
CC	@hfinkel,@zygoloid,@Weverything,@seanm

Extended Description

Code such as this produces no warning

void bar(int x)
{
    if ((x == 0) >> 3)
    {

    }
}

Discovered from problem fixed in r271356

[zygoloid]

It seems very reasonable to warn on "b >> x" where b is of type bool, suggesting rewriting as "b & !x" if that's really the intent.

On the other hand, "b << x" is quite likely to be intentional, so I don't think we can reasonably warn on that case.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[angelomatni]

I'm interested in working on this issue despite being unfamiliar with development on clang diagnostics. I'll try to get up to speed on that in the coming days.

Conceptually, I agree that a warning that suggests "b >> x", for b of type bool, be re-written as "b & !x" prevents unintended bugs like in r271356. I think that "b << x" is tricky but should be tackled because shifting a boolean is probably done unintentionally 99% of the time. Would warning on "b << x" with suggested re-write "b && (x | 31)" seem logical? Is there a cleaner way to express the assumed bool to int conversion?

[shafik]

I think DiagnoseBadShiftValues(...) might be where we want to diagnose this since we are catching other such bad shift values there. I think you would need to do something like LHS->get()->IgnoreImpCasts()->getType()->isBooleanType() or maybe IgnoreParenImpCasts().

[angelomatni]

Will figure out how to properly link this to this issue later, but here's my work in progress. Adding the check to DiagnoseBadShiftValues caused it not to be reached in the unit test I wrote up; I will figure out why later.
https://reviews.llvm.org/D141414

[arsenm]

Will figure out how to properly link this to this issue later, but here's my work in progress. Adding the check to DiagnoseBadShiftValues caused it not to be reached in the unit test I wrote up; I will figure out why later.
https://reviews.llvm.org/D141414

I think You just put "Fixes #28334" in the commit message. It at least works when you push it, don't think Phabricator is scraped for it

[arsenm]

ping? This patch was never pushed and didn't make the move to a PR

[shafik]

@angelomatni if you are not able to finish this up I will label this a good first issue and let someone take a shot at it. Please let me know.