| Message ID | 20190502082953.31769-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: use logical operators with boolean type | expand |
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.
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;
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(-)