Re: [PATCH] drm/i915/dp: use logical operators with boolean type

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 02 May 2019, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> Em qui, 2019-05-02 às 11:29 +0300, Jani Nikula escreveu:
>> Using arithmetic operators with booleans is confusing. Switch to logical
>> operators.
>> 
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 4e7b8d..ef4992f 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5094,7 +5094,7 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
>>  	enum port port = intel_dig_port->base.port;
>>  	enum tc_port_type old_type = intel_dig_port->tc_type;
>>  
>> -	WARN_ON(is_legacy + is_typec + is_tbt != 1);
>> +	WARN_ON(is_legacy || is_typec || !is_tbt);
>
> This changes the meaning. You're interpreting this as:
>
>     WARN_ON(is_legacy + is_typec + (is_tbt != 1))
>
> while the original intent of the code is to be:
>
>     WARN_ON((is_legacy + is_typec + is_tbt) != 1)

*blush*

> and a quick check on operator precedence tables leads me to think the
> original code is indeed correct.
>
> We're asserting exactly one of these bools enabled, so the logic
> operation would be something like:
>
> WARN_ON((is_legacy && (is_typec || is_tbt)) ||
>         (is_typec && (is_legacy || is_tbt)) ||
>  	(is_tbt && (is_legacy || is_typec)) ||
> 	(!is_legacy && !is_typec && !is_tbt))
>
> I would still prefer the arithmetic operation.

Agreed.

I'll go hide under a rock.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx