* Re: [PATCH v2] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all
2019-11-05 4:59 ` [PATCH v2] " Nathan Chancellor
@ 2019-12-06 21:59 ` Nick Desaulniers
2019-12-07 10:41 ` Ezequiel Garcia
2019-12-08 21:11 ` [PATCH v3] media: v4l2-device.h: Explicitly compare grp{id,mask} to zero in v4l2_device macros Nathan Chancellor
2 siblings, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2019-12-06 21:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, LKML, clang-built-linux, Nathan Chancellor
Bumping for review.
On Mon, Nov 4, 2019 at 9:00 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building with Clang + -Wtautological-constant-compare, several of
> the ivtv drivers warn along the lines of:
>
> drivers/media/pci/cx18/cx18-driver.c:1005:21: warning: converting the
> result of '<<' to a boolean always evaluates to true
> [-Wtautological-constant-compare]
> cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
> ^
> drivers/media/pci/cx18/cx18-cards.h:18:37: note: expanded from macro
> 'CX18_HW_GPIO_RESET_CTRL'
> #define CX18_HW_GPIO_RESET_CTRL (1 << 6)
> ^
> 1 warning generated.
>
> This is because the shift operation is implicitly converted to a boolean
> in v4l2_device_mask_call_all before being negated. This can be solved by
> just comparing the mask result to 0 explicitly so that there is no
> boolean conversion.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/752
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2: https://lore.kernel.org/lkml/20191024201240.49063-1-natechancellor@gmail.com/
>
> * Fix typo in commit message
> * Add Nick's Reviewed-by.
>
> include/media/v4l2-device.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> index e0b8f2602670..8564b3227887 100644
> --- a/include/media/v4l2-device.h
> +++ b/include/media/v4l2-device.h
> @@ -431,8 +431,8 @@ static inline bool v4l2_device_supports_requests(struct v4l2_device *v4l2_dev)
> struct v4l2_subdev *__sd; \
> \
> __v4l2_device_call_subdevs_p(v4l2_dev, __sd, \
> - !(grpmsk) || (__sd->grp_id & (grpmsk)), o, f , \
> - ##args); \
> + (grpmsk) == 0 || (__sd->grp_id & (grpmsk)), o, \
> + f , ##args); \
> } while (0)
>
> /**
> --
> 2.24.0
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all
2019-11-05 4:59 ` [PATCH v2] " Nathan Chancellor
2019-12-06 21:59 ` Nick Desaulniers
@ 2019-12-07 10:41 ` Ezequiel Garcia
2019-12-08 21:11 ` [PATCH v3] media: v4l2-device.h: Explicitly compare grp{id,mask} to zero in v4l2_device macros Nathan Chancellor
2 siblings, 0 replies; 6+ messages in thread
From: Ezequiel Garcia @ 2019-12-07 10:41 UTC (permalink / raw)
To: Nathan Chancellor, Mauro Carvalho Chehab, Hans Verkuil
Cc: linux-media, linux-kernel, clang-built-linux, Nick Desaulniers
(Adding Hans)
Hi Nathan, Nick,
Thanks for the patch (and the patience).
On Mon, 2019-11-04 at 21:59 -0700, Nathan Chancellor wrote:
> When building with Clang + -Wtautological-constant-compare, several of
> the ivtv drivers warn along the lines of:
>
Nitpick: s/ivtv/ivtv and cx18
> drivers/media/pci/cx18/cx18-driver.c:1005:21: warning: converting the
> result of '<<' to a boolean always evaluates to true
> [-Wtautological-constant-compare]
> cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
> ^
> drivers/media/pci/cx18/cx18-cards.h:18:37: note: expanded from macro
> 'CX18_HW_GPIO_RESET_CTRL'
> #define CX18_HW_GPIO_RESET_CTRL (1 << 6)
> ^
> 1 warning generated.
>
> This is because the shift operation is implicitly converted to a boolean
> in v4l2_device_mask_call_all before being negated. This can be solved by
> just comparing the mask result to 0 explicitly so that there is no
> boolean conversion.
>
Perhaps it's interesting to mention the reason for this change,
which you mentioned as a comment before:
"""
The ultimate goal is to get
-Wtautological-compare enabled because there are several subwarnings
that would be helpful to have and right now they are all disabled.
"""
> Link: https://github.com/ClangBuiltLinux/linux/issues/752
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2: https://lore.kernel.org/lkml/20191024201240.49063-1-natechancellor@gmail.com/
>
> * Fix typo in commit message
> * Add Nick's Reviewed-by.
>
> include/media/v4l2-device.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> index e0b8f2602670..8564b3227887 100644
> --- a/include/media/v4l2-device.h
> +++ b/include/media/v4l2-device.h
> @@ -431,8 +431,8 @@ static inline bool v4l2_device_supports_requests(struct v4l2_device *v4l2_dev)
> struct v4l2_subdev *__sd; \
> \
> __v4l2_device_call_subdevs_p(v4l2_dev, __sd, \
> - !(grpmsk) || (__sd->grp_id & (grpmsk)), o, f , \
> - ##args); \
> + (grpmsk) == 0 || (__sd->grp_id & (grpmsk)), o, \
> + f , ##args); \
> } while (0)
>
> /**
The change is small enough and if it helps enabling helpful
warnings, then it seems a good idea.
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
For consistency, it would be good to also patch the other case(s).
Thanks,
Ezequiel
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3] media: v4l2-device.h: Explicitly compare grp{id,mask} to zero in v4l2_device macros
2019-11-05 4:59 ` [PATCH v2] " Nathan Chancellor
2019-12-06 21:59 ` Nick Desaulniers
2019-12-07 10:41 ` Ezequiel Garcia
@ 2019-12-08 21:11 ` Nathan Chancellor
2 siblings, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-12-08 21:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Hans Verkuil
Cc: linux-media, linux-kernel, Nathan Chancellor, Ezequiel Garcia,
Nick Desaulniers
When building with Clang + -Wtautological-constant-compare, several of
the ivtv and cx18 drivers warn along the lines of:
drivers/media/pci/cx18/cx18-driver.c:1005:21: warning: converting the
result of '<<' to a boolean always evaluates to true
[-Wtautological-constant-compare]
cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
^
drivers/media/pci/cx18/cx18-cards.h:18:37: note: expanded from macro
'CX18_HW_GPIO_RESET_CTRL'
#define CX18_HW_GPIO_RESET_CTRL (1 << 6)
^
1 warning generated.
This warning happens because the shift operation is implicitly converted
to a boolean in v4l2_device_mask_call_all before being negated. This can
be solved by just comparing the mask result to 0 explicitly so that
there is no boolean conversion. The ultimate goal is to enable
-Wtautological-compare globally because there are several subwarnings
that would be helpful to have.
For visual consistency and avoidance of these warnings in the future,
all of the implicitly boolean conversions in the v4l2_device macros
are converted to explicit ones as well.
Link: https://github.com/ClangBuiltLinux/linux/issues/752
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2: https://lore.kernel.org/lkml/20191024201240.49063-1-natechancellor@gmail.com/
* Fix typo in commit message
* Add Nick's Reviewed-by.
v2 -> v3: https://lore.kernel.org/lkml/20191105045907.26123-1-natechancellor@gmail.com/
* Improve reasoning for change (Ezequiel)
* Patch all implicit boolean conversions (Ezequiel)
* Add Ezequiel's reviewed-by.
include/media/v4l2-device.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 5f36e0d2ede6..95353ae476a1 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -371,7 +371,7 @@ static inline bool v4l2_device_supports_requests(struct v4l2_device *v4l2_dev)
struct v4l2_subdev *__sd; \
\
__v4l2_device_call_subdevs_p(v4l2_dev, __sd, \
- !(grpid) || __sd->grp_id == (grpid), o, f , \
+ (grpid) == 0 || __sd->grp_id == (grpid), o, f , \
##args); \
} while (0)
@@ -403,7 +403,7 @@ static inline bool v4l2_device_supports_requests(struct v4l2_device *v4l2_dev)
({ \
struct v4l2_subdev *__sd; \
__v4l2_device_call_subdevs_until_err_p(v4l2_dev, __sd, \
- !(grpid) || __sd->grp_id == (grpid), o, f , \
+ (grpid) == 0 || __sd->grp_id == (grpid), o, f , \
##args); \
})
@@ -431,8 +431,8 @@ static inline bool v4l2_device_supports_requests(struct v4l2_device *v4l2_dev)
struct v4l2_subdev *__sd; \
\
__v4l2_device_call_subdevs_p(v4l2_dev, __sd, \
- !(grpmsk) || (__sd->grp_id & (grpmsk)), o, f , \
- ##args); \
+ (grpmsk) == 0 || (__sd->grp_id & (grpmsk)), o, \
+ f , ##args); \
} while (0)
/**
@@ -462,8 +462,8 @@ static inline bool v4l2_device_supports_requests(struct v4l2_device *v4l2_dev)
({ \
struct v4l2_subdev *__sd; \
__v4l2_device_call_subdevs_until_err_p(v4l2_dev, __sd, \
- !(grpmsk) || (__sd->grp_id & (grpmsk)), o, f , \
- ##args); \
+ (grpmsk) == 0 || (__sd->grp_id & (grpmsk)), o, \
+ f , ##args); \
})
--
2.24.0
^ permalink raw reply [flat|nested] 6+ messages in thread