Hi Sakari,
On Thursday 18 May 2017 23:50:34 Sakari Ailus wrote:
> On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> > On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> >> On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> >>> From: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> >>>
> >>> Return NULL, if a null entity is parsed for it's v4l2_subdev
> >>>
> >>> Signed-off-by: Kieran Bingham
> >>> <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> >>> ---
> >>>
> >>> include/media/v4l2-subdev.h | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>> index 5f1669c45642..72d7f28f38dc 100644
> >>> --- a/include/media/v4l2-subdev.h
> >>> +++ b/include/media/v4l2-subdev.h
> >>> @@ -829,7 +829,7 @@ struct v4l2_subdev {
> >>> };
> >>>
> >>> #define media_entity_to_v4l2_subdev(ent) \
> >>> - container_of(ent, struct v4l2_subdev, entity)
> >>> + (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> >>> #define vdev_to_v4l2_subdev(vdev) \
> >>> ((struct v4l2_subdev *)video_get_drvdata(vdev))
> >>
> >> The problem with this is that ent is now referenced twice. If the ent
> >> macro argument has side effect, this would introduce bugs. It's
> >> unlikely, but worth avoiding. Either use a macro or a function.
> >>
> >> I think I'd use function for there's little use for supporting for const
> >> and non-const arguments presumably. A simple static inline function
> >> should do.
> >
> > Note that, if we want to keep using a macro, this could be written as
> >
> > #define media_entity_to_v4l2_subdev(ent) ({ \
> >
> > typeof(ent) __ent = ent; \
I just realized that this should be written
typeof(ent) __ent = (ent);
> > __ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
> >
> > })
> >
> > Bonus point if you can come up with a way to return a const struct
> > v4l2_subdev pointer when then ent argument is const.
>
> I can't think of a use case for that. I've never seen a const struct
> v4l2_subdev anywhere. I could be just oblivious though. :-)
I agree with you, it's overkill, at least for now. Although I'd like to see
how it could be done, for other similar constructs where both const and non-
const versions are useful.
> Better give a __ent a name that someone will not accidentally come up with.
> That can lead to problems that are difficult to debug --- for the code
> compiles, it just doesn't do what's expected.
Won't it generate a compilation error as the variable would be redefined by
the macro ?
--
Regards,
Laurent Pinchart
