Re: [PATCH] platform/x86: thinkpad_acpi: Fix bitwise vs. logical warning

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>,
	Henrique de Moraes Holschuh <hmh@hmh.eng.br>,
	 Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <markgross@kernel.org>,
	 ibm-acpi-devel@lists.sourceforge.net,
	platform-driver-x86@vger.kernel.org,
	 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	llvm@lists.linux.dev,  Tor Vic <torvic9@mailbox.org>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Fix bitwise vs. logical warning
Date: Mon, 18 Oct 2021 17:38:09 -1000	[thread overview]
Message-ID: <CAHk-=wjwOnrHXDSqnmhiKujk=5XieJFvfnQwc2WKOKFwzcqvaA@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOd=wGjd_L1703Y9Kngcr9-_wTvcRLToiydXYkR=S_9xWDw@mail.gmail.com>

On Mon, Oct 18, 2021 at 10:14 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Right, the patch that added the warning explicitly checks for side effects.

Well, it's a bit questionable. The "side effects" are things like any
pointer dereference, because it could fault, but if you know that
isn't an issue, then clang basically ends up complaining about code
that is perfectly fine. Maybe it was written that way on purpose, like
the kvm code.

Now, it's probably not worth keeping that "bitops of booleans" logic -
if it is a noticeable optimization, it's generally something that the
compiler should do for us, but basically clang is warning about
perfectly valid code.

And what I find absolutely disgusting is the suggested "fix" that
clang gives you.

If the warning said "maybe you meant to use a logical or (||)", then
that would be one thing. But what clang suggests as the "fix" for the
warning is just bad coding practice.

If a warning fix involves making the code uglier, then the warning fix is wrong.

This is not the first time we've had compilers suggesting garbage. Gcc
used to suggest (perhaps still does) the "extra parenthesis" for
"assignment used as a truth value" situation. Which is - once again -
disgusting garbage.

Writing code like

        if (a = b) ..

is bad and error prone. But the suggestion to "fix" the warning with

        if ((a = b)) ..

is just completely unacceptably stupid, and is just BAD CODE.

The proper fix might be to write it like

        if ((a = b) != 0) ...

which at least makes the truth value part explicit - in ways that a
silly double parenthesis does not. Or, better yet, write it as

        a = b;
        if (a) ..

instead, which is legible and fine.

The clang suggestion to add a cast to 'int' to avoid the warning is
the same kind of "write bad code" suggestion. Just don't do it.

             Linus