* [PATCH] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all
@ 2019-10-24 20:12 Nathan Chancellor
2019-11-04 19:07 ` Nick Desaulniers
2019-11-05 4:59 ` [PATCH v2] " Nathan Chancellor
0 siblings, 2 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-10-24 20:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, clang-built-linux, Nathan Chancellor
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 explicity so that there is no
boolean conversion.
Link: https://github.com/ClangBuiltLinux/linux/issues/752
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
I am aware that there is suddenly a style mismatch (some macros using
!(grpmask) and this one using (grpmask) == 0) but I chose to display the
minimal fix. If you want me to update all the macros to use this style,
I'd be happy to in a followup patch.
There are 19 of these warnings in the drivers/media/pci folder, which
can be seen here:
https://github.com/ClangBuiltLinux/linux/issues/488#issuecomment-545218125
This is the simplest fix but if you all prefer an alternative one, I
would be happy to see/review/test it. 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:
https://github.com/ClangBuiltLinux/linux/issues/488
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.rc1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all
2019-10-24 20:12 [PATCH] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all Nathan Chancellor
@ 2019-11-04 19:07 ` Nick Desaulniers
2019-11-05 4:59 ` [PATCH v2] " Nathan Chancellor
1 sibling, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2019-11-04 19:07 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Mauro Carvalho Chehab, linux-media, LKML, clang-built-linux
On Thu, Oct 24, 2019 at 1:17 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 explicity so that there is no
s/explicity/explicitly/
Harmless enough change, thanks for the patch, and sorry for taking so
long to review.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> boolean conversion.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/752
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> I am aware that there is suddenly a style mismatch (some macros using
> !(grpmask) and this one using (grpmask) == 0) but I chose to display the
> minimal fix. If you want me to update all the macros to use this style,
> I'd be happy to in a followup patch.
>
> There are 19 of these warnings in the drivers/media/pci folder, which
> can be seen here:
>
> https://github.com/ClangBuiltLinux/linux/issues/488#issuecomment-545218125
>
> This is the simplest fix but if you all prefer an alternative one, I
> would be happy to see/review/test it. 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:
>
> https://github.com/ClangBuiltLinux/linux/issues/488
>
> 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)
>
> /**
> --
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all
2019-10-24 20:12 [PATCH] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all Nathan Chancellor
2019-11-04 19:07 ` Nick Desaulniers
@ 2019-11-05 4:59 ` Nathan Chancellor
2019-12-06 21:59 ` Nick Desaulniers
` (2 more replies)
1 sibling, 3 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-11-05 4:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, clang-built-linux, Nathan Chancellor,
Nick Desaulniers
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
^ permalink raw reply related [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: 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 related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-08 21:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-24 20:12 [PATCH] media: v4l2-device.h: Explicitly compare grpmask to zero in v4l2_device_mask_call_all Nathan Chancellor
2019-11-04 19:07 ` Nick Desaulniers
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.