Re: [PATCH v5 02/12] HID: pidff: Do not send effect envelope if it's empty

[Oliver Neukum <oneukum@suse.com>]

On 21.01.25 11:17, Tomasz Pakuła wrote:
> On Tue, 21 Jan 2025 at 10:59, Oliver Neukum <oneukum@suse.com> wrote:
>> I am afraid this is the most convoluted piece of boolean algebra I've seen
>> in a long time. In particular because it mixes things that do not belong together.
> 
> Could you elaborate on that? What here does not belong?

Hi,

> 
> I think the diff is a bit unfortunate and doesn't make it justice, but
> this is based on
> code that was already there. Instead of just checking if the new
> values differ from
> old values, we first check if any values are different from 0. If
> neither are != 0 OR
> the effect didn't contain an envelope previously (NULL here), we
> return the value
> of the check.

Indeed. And that is the problem.
You could see the evaluation to contain either two or three main cases.

First view:

A - everything is 0. We return false.
B - we compare the old and the new and return the comparison. With a subcase
of no old old values existing.

In the first view you are mixing the test for no old values of the second case
with the test for no old values existing.

Or you split up the second condition into two independent cases.


[..]
> 
>          if (!needs_new_envelope || !old)
>                  return needs_new_envelope;

This boolean statement stems from a common result, not from a common
logical reason for acting so. This is clear because if the first half
is true, you are returning itself.

This statement would be so much more clear as:

if (!needs_new_envelope)
	return false;

if (!old)
	return needs_new_envelope;

	Regards
		Oliver