drm/i915/dp: use logical operators with boolean type

Submitted by Jani Nikula on May 2, 2019, 8:29 a.m.

Details

Message ID 20190502082953.31769-1-jani.nikula@intel.com
State New
Headers show
Series "drm/i915/dp: use logical operators with boolean type" ( rev: 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula May 2, 2019, 8:29 a.m.
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(-)

Patch hide | download patch | download mbox

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);
 
 	if (is_legacy)
 		intel_dig_port->tc_type = TC_PORT_LEGACY;

Comments

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)

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.    

>  
>  	if (is_legacy)
>  		intel_dig_port->tc_type = TC_PORT_LEGACY;
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.