4 * redundant conditions

[dcb]

1.

[qemu/hw/block/nvme.c:355]: (style) Redundant condition: sqid. 'A && (!A || B)' is equivalent to 'A || B'

  if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {

2.

[qemu/hw/block/nvme.c:429]: (style) Redundant condition: cqid. 'A && (!A || B)' is equivalent to 'A || B'

  if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {

3.

[qemu/hw/tpm/tpm_passthrough.c:157]: (style) Redundant condition: tpm_pt.tpm_op_canceled. 'A && (!A || B)' is equivalent to 'A || B'

     if (!tpm_pt->tpm_op_canceled ||
            (tpm_pt->tpm_op_canceled && errno != ECANCELED)) {

4.

[qemu/target-arm/translate-a64.c:5729]: (style) Redundant condition: size<3. 'A && (!A || B)' is equivalent to 'A || B'

      if (size > 3
            || (size < 3 && is_q)
            || (size == 3 && !is_q)) {

[Peter Maydell (pmaydell)]

On 12 June 2015 at 11:38, dcb <email address hidden> wrote:
> Public bug reported:
>
>
> 1.
>
> [qemu/hw/block/nvme.c:355]: (style) Redundant condition: sqid. 'A && (!A
> || B)' is equivalent to 'A || B'
>
> if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
>
> 2.
>
> [qemu/hw/block/nvme.c:429]: (style) Redundant condition: cqid. 'A && (!A
> || B)' is equivalent to 'A || B'
>
> if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
>
> 3.
>
> [qemu/hw/tpm/tpm_passthrough.c:157]: (style) Redundant condition:
> tpm_pt.tpm_op_canceled. 'A && (!A || B)' is equivalent to 'A || B'
>
> if (!tpm_pt->tpm_op_canceled ||
> (tpm_pt->tpm_op_canceled && errno != ECANCELED)) {

These three are all straightforward and would look simpler
in their simplified versions...

> 4.
>
> [qemu/target-arm/translate-a64.c:5729]: (style) Redundant condition:
> size<3. 'A && (!A || B)' is equivalent to 'A || B'
>
> if (size > 3
> || (size < 3 && is_q)
> || (size == 3 && !is_q)) {

...but I'm less sure about this one. I'm not even sure
what it's trying to suggest this should simplify to:
just dropping "size < 3" is obviously wrong, and the
condition format isn't "A && (!A || B)" either.

thanks
-- PMM

[Eric Blake (eblake)]

On 06/12/2015 05:01 AM, Peter Maydell wrote:

>> 4.
>>
>> [qemu/target-arm/translate-a64.c:5729]: (style) Redundant condition:
>> size<3. 'A && (!A || B)' is equivalent to 'A || B'
>>
>> if (size > 3
>> || (size < 3 && is_q)
>> || (size == 3 && !is_q)) {
>
> ...but I'm less sure about this one. I'm not even sure
> what it's trying to suggest this should simplify to:
> just dropping "size < 3" is obviously wrong, and the
> condition format isn't "A && (!A || B)" either.

Let's break it down into the 6 possibilities based on the binary *
ternary conditions being checked:

> 3, is_q => accept
> 3, !is_q => accept
== 3, is_q => reject
== 3, !is_q => accept
< 3, is_q => accept
< 3, !is_q => reject

Here's a shorter conditional with the same properties, but it's gross:

if (size > 3 || (is_q != (size == 3))) {

Too much mental thought to prove it accepts the same set of conditions.

--
Eric Blake eblake redhat com [phone]
Libvirt virtualization library http://libvirt.org

[Peter Maydell (pmaydell)]

On 12 June 2015 at 14:03, Eric Blake <email address hidden> wrote:
> On 06/12/2015 05:01 AM, Peter Maydell wrote:
>
>>> 4.
>>>
>>> [qemu/target-arm/translate-a64.c:5729]: (style) Redundant condition:
>>> size<3. 'A && (!A || B)' is equivalent to 'A || B'
>>>
>>> if (size > 3
>>> || (size < 3 && is_q)
>>> || (size == 3 && !is_q)) {
>>
>> ...but I'm less sure about this one. I'm not even sure
>> what it's trying to suggest this should simplify to:
>> just dropping "size < 3" is obviously wrong, and the
>> condition format isn't "A && (!A || B)" either.
>
> Let's break it down into the 6 possibilities based on the binary *
> ternary conditions being checked:
>
>> 3, is_q => accept
>> 3, !is_q => accept
> == 3, is_q => reject
> == 3, !is_q => accept
> < 3, is_q => accept
> < 3, !is_q => reject
>
> Here's a shorter conditional with the same properties, but it's gross:
>
> if (size > 3 || (is_q != (size == 3))) {
>
> Too much mental thought to prove it accepts the same set of conditions.

Yeah, I think this is the kind of thing where I say "the compiler
should do this simplification if it cares enough" :-)

-- PMM

[dcb (dcb314)]

>These three are all straightforward and would look simpler
>in their simplified versions...

Agreed. The first 3 look valid candidates for simplification.

> 4.
>
> [qemu/target-arm/translate-a64.c:5729]: (style) Redundant condition:
> size<3. 'A && (!A || B)' is equivalent to 'A || B'
>
> if (size > 3
> || (size < 3 && is_q)
> || (size == 3 && !is_q)) {

>...but I'm less sure about this one.

Me too. Suggest regard as a false positive from the static analysis tool
and so leave the original code alone.

[Thomas Huth (th-huth)]

Patches have been committed:
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=f96fe6b5c27b9a66dba71
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=5f333d79a4337b390fa41

[Thomas Huth (th-huth)]

Released with version 2.8