* [PATCH] media: cedrus: don't initialize pointers with zero @ 2018-12-07 10:56 Mauro Carvalho Chehab 2018-12-07 11:14 ` Hans Verkuil 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2018-12-07 10:56 UTC (permalink / raw) Cc: Mauro Carvalho Chehab, Linux Media Mailing List, Mauro Carvalho Chehab, Maxime Ripard, Paul Kocialkowski, Greg Kroah-Hartman, Chen-Yu Tsai, devel, linux-arm-kernel A common mistake is to assume that initializing a var with: struct foo f = { 0 }; Would initialize a zeroed struct. Actually, what this does is to initialize the first element of the struct to zero. According to C99 Standard 6.7.8.21: "If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration." So, in practice, it could zero the entire struct, but, if the first element is not an integer, it will produce warnings: drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer A proper way to initialize it with gcc is to use: struct foo f = { }; But that seems to be a gcc extension. So, I decided to check upstream what's the most commonly pattern. The gcc pattern has about 2000 entries: $ git grep -E "=\s*\{\s*\}"|wc -l 1951 The standard-C compliant pattern has about 2500 entries: $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l 137 $ git grep -E "=\s*\{\s*0\s*\}"|wc -l 2323 So, let's initialize those structs with: = { NULL } Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> --- drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c index b538eb0321d8..44c45c687503 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx) memset(ctx->ctrls, 0, ctrl_size); for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) { - struct v4l2_ctrl_config cfg = { 0 }; + struct v4l2_ctrl_config cfg = { NULL }; cfg.elem_size = cedrus_controls[i].elem_size; cfg.id = cedrus_controls[i].id; diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index e40180a33951..4099a42dba2d 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv) { struct cedrus_ctx *ctx = priv; struct cedrus_dev *dev = ctx->dev; - struct cedrus_run run = { 0 }; + struct cedrus_run run = { NULL }; struct media_request *src_req; unsigned long flags; -- 2.19.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] media: cedrus: don't initialize pointers with zero 2018-12-07 10:56 [PATCH] media: cedrus: don't initialize pointers with zero Mauro Carvalho Chehab @ 2018-12-07 11:14 ` Hans Verkuil 2018-12-07 11:31 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Hans Verkuil @ 2018-12-07 11:14 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Maxime Ripard, Paul Kocialkowski, Greg Kroah-Hartman, Chen-Yu Tsai, devel, linux-arm-kernel On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote: > A common mistake is to assume that initializing a var with: > struct foo f = { 0 }; > > Would initialize a zeroed struct. Actually, what this does is > to initialize the first element of the struct to zero. > > According to C99 Standard 6.7.8.21: > > "If there are fewer initializers in a brace-enclosed > list than there are elements or members of an aggregate, > or fewer characters in a string literal used to initialize > an array of known size than there are elements in the array, > the remainder of the aggregate shall be initialized implicitly > the same as objects that have static storage duration." > > So, in practice, it could zero the entire struct, but, if the > first element is not an integer, it will produce warnings: > > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer > > A proper way to initialize it with gcc is to use: > > struct foo f = { }; > > But that seems to be a gcc extension. So, I decided to check upstream No, this is not a gcc extension. It's part of the latest C standard. It's used all over in the kernel, so please use {} instead of { NULL }. Personally I think {} is a fantastic invention and I wish C++ had it as well. Regards, Hans > what's the most commonly pattern. The gcc pattern has about 2000 entries: > > $ git grep -E "=\s*\{\s*\}"|wc -l > 1951 > > The standard-C compliant pattern has about 2500 entries: > > $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l > 137 > $ git grep -E "=\s*\{\s*0\s*\}"|wc -l > 2323 > > So, let's initialize those structs with: > = { NULL } > > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > --- > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c > index b538eb0321d8..44c45c687503 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx) > memset(ctx->ctrls, 0, ctrl_size); > > for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) { > - struct v4l2_ctrl_config cfg = { 0 }; > + struct v4l2_ctrl_config cfg = { NULL }; > > cfg.elem_size = cedrus_controls[i].elem_size; > cfg.id = cedrus_controls[i].id; > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > index e40180a33951..4099a42dba2d 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv) > { > struct cedrus_ctx *ctx = priv; > struct cedrus_dev *dev = ctx->dev; > - struct cedrus_run run = { 0 }; > + struct cedrus_run run = { NULL }; > struct media_request *src_req; > unsigned long flags; > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: cedrus: don't initialize pointers with zero 2018-12-07 11:14 ` Hans Verkuil @ 2018-12-07 11:31 ` Mauro Carvalho Chehab 2018-12-07 11:37 ` Hans Verkuil 2018-12-07 13:22 ` Dan Carpenter 0 siblings, 2 replies; 7+ messages in thread From: Mauro Carvalho Chehab @ 2018-12-07 11:31 UTC (permalink / raw) To: Hans Verkuil Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Maxime Ripard, Paul Kocialkowski, Greg Kroah-Hartman, Chen-Yu Tsai, devel, linux-arm-kernel Em Fri, 7 Dec 2018 12:14:50 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote: > > A common mistake is to assume that initializing a var with: > > struct foo f = { 0 }; > > > > Would initialize a zeroed struct. Actually, what this does is > > to initialize the first element of the struct to zero. > > > > According to C99 Standard 6.7.8.21: > > > > "If there are fewer initializers in a brace-enclosed > > list than there are elements or members of an aggregate, > > or fewer characters in a string literal used to initialize > > an array of known size than there are elements in the array, > > the remainder of the aggregate shall be initialized implicitly > > the same as objects that have static storage duration." > > > > So, in practice, it could zero the entire struct, but, if the > > first element is not an integer, it will produce warnings: > > > > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer > > > > A proper way to initialize it with gcc is to use: > > > > struct foo f = { }; > > > > But that seems to be a gcc extension. So, I decided to check upstream > > No, this is not a gcc extension. It's part of the latest C standard. Sure? Where the C standard spec states that? I've been seeking for such info for a while, as '= {}' is also my personal preference. I tried to build the Kernel with clang, just to be sure that this won't cause issues with the clang support, but, unfortunately, the clang compiler (not even the upstream version) is able to build the upstream Kernel yet, as it lacks asm-goto support (there is an OOT patch for llvm solving it - but it sounds too much effort for my time to have to build llvm from their sources just for a simple check like this). > It's used all over in the kernel, so please use {} instead of { NULL }. > > Personally I think {} is a fantastic invention and I wish C++ had it as > well. Fully agreed on that. > > Regards, > > Hans > > > what's the most commonly pattern. The gcc pattern has about 2000 entries: > > > > $ git grep -E "=\s*\{\s*\}"|wc -l > > 1951 > > > > The standard-C compliant pattern has about 2500 entries: > > > > $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l > > 137 > > $ git grep -E "=\s*\{\s*0\s*\}"|wc -l > > 2323 > > > > So, let's initialize those structs with: > > = { NULL } > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > > --- > > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c > > index b538eb0321d8..44c45c687503 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx) > > memset(ctx->ctrls, 0, ctrl_size); > > > > for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) { > > - struct v4l2_ctrl_config cfg = { 0 }; > > + struct v4l2_ctrl_config cfg = { NULL }; > > > > cfg.elem_size = cedrus_controls[i].elem_size; > > cfg.id = cedrus_controls[i].id; > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > index e40180a33951..4099a42dba2d 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv) > > { > > struct cedrus_ctx *ctx = priv; > > struct cedrus_dev *dev = ctx->dev; > > - struct cedrus_run run = { 0 }; > > + struct cedrus_run run = { NULL }; > > struct media_request *src_req; > > unsigned long flags; > > > > > Thanks, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: cedrus: don't initialize pointers with zero 2018-12-07 11:31 ` Mauro Carvalho Chehab @ 2018-12-07 11:37 ` Hans Verkuil 2018-12-07 12:27 ` Ian Arkver 2018-12-07 13:22 ` Dan Carpenter 1 sibling, 1 reply; 7+ messages in thread From: Hans Verkuil @ 2018-12-07 11:37 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Maxime Ripard, Paul Kocialkowski, Greg Kroah-Hartman, Chen-Yu Tsai, devel, linux-arm-kernel On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote: > Em Fri, 7 Dec 2018 12:14:50 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote: >>> A common mistake is to assume that initializing a var with: >>> struct foo f = { 0 }; >>> >>> Would initialize a zeroed struct. Actually, what this does is >>> to initialize the first element of the struct to zero. >>> >>> According to C99 Standard 6.7.8.21: >>> >>> "If there are fewer initializers in a brace-enclosed >>> list than there are elements or members of an aggregate, >>> or fewer characters in a string literal used to initialize >>> an array of known size than there are elements in the array, >>> the remainder of the aggregate shall be initialized implicitly >>> the same as objects that have static storage duration." >>> >>> So, in practice, it could zero the entire struct, but, if the >>> first element is not an integer, it will produce warnings: >>> >>> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer >>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer >>> >>> A proper way to initialize it with gcc is to use: >>> >>> struct foo f = { }; >>> >>> But that seems to be a gcc extension. So, I decided to check upstream >> >> No, this is not a gcc extension. It's part of the latest C standard. > > Sure? Where the C standard spec states that? I've been seeking for > such info for a while, as '= {}' is also my personal preference. I believe it was added in C11, but I've loaned the book that stated that to someone else, so I can't check. In any case, it's used everywhere: git grep '= *{ *}' drivers/ So it is really pointless to *not* use it. Regards, Hans > I tried to build the Kernel with clang, just to be sure that this > won't cause issues with the clang support, but, unfortunately, the > clang compiler (not even the upstream version) is able to build > the upstream Kernel yet, as it lacks asm-goto support (there is an > OOT patch for llvm solving it - but it sounds too much effort for > my time to have to build llvm from their sources just for a simple > check like this). > >> It's used all over in the kernel, so please use {} instead of { NULL }. >> >> Personally I think {} is a fantastic invention and I wish C++ had it as >> well. > > Fully agreed on that. > >> >> Regards, >> >> Hans >> >>> what's the most commonly pattern. The gcc pattern has about 2000 entries: >>> >>> $ git grep -E "=\s*\{\s*\}"|wc -l >>> 1951 >>> >>> The standard-C compliant pattern has about 2500 entries: >>> >>> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l >>> 137 >>> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l >>> 2323 >>> >>> So, let's initialize those structs with: >>> = { NULL } >>> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> >>> --- >>> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- >>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c >>> index b538eb0321d8..44c45c687503 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c >>> @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx) >>> memset(ctx->ctrls, 0, ctrl_size); >>> >>> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) { >>> - struct v4l2_ctrl_config cfg = { 0 }; >>> + struct v4l2_ctrl_config cfg = { NULL }; >>> >>> cfg.elem_size = cedrus_controls[i].elem_size; >>> cfg.id = cedrus_controls[i].id; >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>> index e40180a33951..4099a42dba2d 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>> @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv) >>> { >>> struct cedrus_ctx *ctx = priv; >>> struct cedrus_dev *dev = ctx->dev; >>> - struct cedrus_run run = { 0 }; >>> + struct cedrus_run run = { NULL }; >>> struct media_request *src_req; >>> unsigned long flags; >>> >>> >> > > > > Thanks, > Mauro > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: cedrus: don't initialize pointers with zero 2018-12-07 11:37 ` Hans Verkuil @ 2018-12-07 12:27 ` Ian Arkver 2018-12-07 12:58 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Ian Arkver @ 2018-12-07 12:27 UTC (permalink / raw) To: Hans Verkuil, Mauro Carvalho Chehab Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Maxime Ripard, Paul Kocialkowski, Greg Kroah-Hartman, Chen-Yu Tsai, devel, linux-arm-kernel On 07/12/2018 11:37, Hans Verkuil wrote: > On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote: >> Em Fri, 7 Dec 2018 12:14:50 +0100 >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >> >>> On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote: >>>> A common mistake is to assume that initializing a var with: >>>> struct foo f = { 0 }; >>>> >>>> Would initialize a zeroed struct. Actually, what this does is >>>> to initialize the first element of the struct to zero. >>>> >>>> According to C99 Standard 6.7.8.21: >>>> >>>> "If there are fewer initializers in a brace-enclosed >>>> list than there are elements or members of an aggregate, >>>> or fewer characters in a string literal used to initialize >>>> an array of known size than there are elements in the array, >>>> the remainder of the aggregate shall be initialized implicitly >>>> the same as objects that have static storage duration." >>>> >>>> So, in practice, it could zero the entire struct, but, if the >>>> first element is not an integer, it will produce warnings: >>>> >>>> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer >>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer >>>> >>>> A proper way to initialize it with gcc is to use: >>>> >>>> struct foo f = { }; >>>> >>>> But that seems to be a gcc extension. So, I decided to check upstream >>> >>> No, this is not a gcc extension. It's part of the latest C standard. >> >> Sure? Where the C standard spec states that? I've been seeking for >> such info for a while, as '= {}' is also my personal preference. > > I believe it was added in C11, but I've loaned the book that stated > that to someone else, so I can't check. Sadly I don't see mention of empty initializer lists in section 6.7.9 of ISO/IEC 9899:2011, though I've only got a draft version. I had a play with Compiler Explorer[1] and it seems like clang is OK with the {} form though just out of interest msvc isn't. I didn't try exploring other command line options. [1] https://gcc.godbolt.org/z/XIDC7W Regards, Ian > > In any case, it's used everywhere: > > git grep '= *{ *}' drivers/ > > So it is really pointless to *not* use it. > > Regards, > > Hans > >> I tried to build the Kernel with clang, just to be sure that this >> won't cause issues with the clang support, but, unfortunately, the >> clang compiler (not even the upstream version) is able to build >> the upstream Kernel yet, as it lacks asm-goto support (there is an >> OOT patch for llvm solving it - but it sounds too much effort for >> my time to have to build llvm from their sources just for a simple >> check like this). >> >>> It's used all over in the kernel, so please use {} instead of { NULL }. >>> >>> Personally I think {} is a fantastic invention and I wish C++ had it as >>> well. >> >> Fully agreed on that. >> >>> >>> Regards, >>> >>> Hans >>> >>>> what's the most commonly pattern. The gcc pattern has about 2000 entries: >>>> >>>> $ git grep -E "=\s*\{\s*\}"|wc -l >>>> 1951 >>>> >>>> The standard-C compliant pattern has about 2500 entries: >>>> >>>> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l >>>> 137 >>>> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l >>>> 2323 >>>> >>>> So, let's initialize those structs with: >>>> = { NULL } >>>> >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> >>>> --- >>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- >>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c >>>> index b538eb0321d8..44c45c687503 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c >>>> @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx) >>>> memset(ctx->ctrls, 0, ctrl_size); >>>> >>>> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) { >>>> - struct v4l2_ctrl_config cfg = { 0 }; >>>> + struct v4l2_ctrl_config cfg = { NULL }; >>>> >>>> cfg.elem_size = cedrus_controls[i].elem_size; >>>> cfg.id = cedrus_controls[i].id; >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>>> index e40180a33951..4099a42dba2d 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>>> @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv) >>>> { >>>> struct cedrus_ctx *ctx = priv; >>>> struct cedrus_dev *dev = ctx->dev; >>>> - struct cedrus_run run = { 0 }; >>>> + struct cedrus_run run = { NULL }; >>>> struct media_request *src_req; >>>> unsigned long flags; >>>> >>>> >>> >> >> >> >> Thanks, >> Mauro >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: cedrus: don't initialize pointers with zero 2018-12-07 12:27 ` Ian Arkver @ 2018-12-07 12:58 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 7+ messages in thread From: Mauro Carvalho Chehab @ 2018-12-07 12:58 UTC (permalink / raw) To: Ian Arkver Cc: Hans Verkuil, Linux Media Mailing List, Mauro Carvalho Chehab, Maxime Ripard, Paul Kocialkowski, Greg Kroah-Hartman, Chen-Yu Tsai, devel, linux-arm-kernel Em Fri, 7 Dec 2018 12:27:09 +0000 Ian Arkver <ian.arkver.dev@gmail.com> escreveu: > On 07/12/2018 11:37, Hans Verkuil wrote: > > On 12/07/2018 12:31 PM, Mauro Carvalho Chehab wrote: > >> Em Fri, 7 Dec 2018 12:14:50 +0100 > >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> > >>> On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote: > >>>> A common mistake is to assume that initializing a var with: > >>>> struct foo f = { 0 }; > >>>> > >>>> Would initialize a zeroed struct. Actually, what this does is > >>>> to initialize the first element of the struct to zero. > >>>> > >>>> According to C99 Standard 6.7.8.21: > >>>> > >>>> "If there are fewer initializers in a brace-enclosed > >>>> list than there are elements or members of an aggregate, > >>>> or fewer characters in a string literal used to initialize > >>>> an array of known size than there are elements in the array, > >>>> the remainder of the aggregate shall be initialized implicitly > >>>> the same as objects that have static storage duration." > >>>> > >>>> So, in practice, it could zero the entire struct, but, if the > >>>> first element is not an integer, it will produce warnings: > >>>> > >>>> drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer > >>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer > >>>> > >>>> A proper way to initialize it with gcc is to use: > >>>> > >>>> struct foo f = { }; > >>>> > >>>> But that seems to be a gcc extension. So, I decided to check upstream > >>> > >>> No, this is not a gcc extension. It's part of the latest C standard. > >> > >> Sure? Where the C standard spec states that? I've been seeking for > >> such info for a while, as '= {}' is also my personal preference. > > > > I believe it was added in C11, but I've loaned the book that stated > > that to someone else, so I can't check. > > Sadly I don't see mention of empty initializer lists in section 6.7.9 of > ISO/IEC 9899:2011, though I've only got a draft version. Yeah, as far as I checked, this is not really part of the standard. Depending on how you read: "If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration." One may infere that a brace-enclosed list with zero elements would fit, and "the remainder of the aggregate shall be initialized implicitly". I suspect that this is how gcc people interpreted it. > I had a play with Compiler Explorer[1] and it seems like clang is OK > with the {} form though just out of interest msvc isn't. Yeah, I'm aware that msvc won't support it. Good to know that clang does the right thing cleaning the struct. To be realistic, we only really care with gcc at the Kernel side, as clang upstream versions still can't build upstream Kernels, and nobody uses any other compiler for the Kernel anymore. Yet, with regards to clang, there's a push to let it to build the Kernel. So, it seems wise to add something that would work for both. Anyway, I'll post a version 2 of this patch using ={} and placing some rationale on it. > I didn't try > exploring other command line options. > > [1] https://gcc.godbolt.org/z/XIDC7W > > Regards, > Ian > > > > In any case, it's used everywhere: > > > > git grep '= *{ *}' drivers/ > > > > So it is really pointless to *not* use it. > > > > Regards, > > > > Hans > > > >> I tried to build the Kernel with clang, just to be sure that this > >> won't cause issues with the clang support, but, unfortunately, the > >> clang compiler (not even the upstream version) is able to build > >> the upstream Kernel yet, as it lacks asm-goto support (there is an > >> OOT patch for llvm solving it - but it sounds too much effort for > >> my time to have to build llvm from their sources just for a simple > >> check like this). > >> > >>> It's used all over in the kernel, so please use {} instead of { NULL }. > >>> > >>> Personally I think {} is a fantastic invention and I wish C++ had it as > >>> well. > >> > >> Fully agreed on that. > >> > >>> > >>> Regards, > >>> > >>> Hans > >>> > >>>> what's the most commonly pattern. The gcc pattern has about 2000 entries: > >>>> > >>>> $ git grep -E "=\s*\{\s*\}"|wc -l > >>>> 1951 > >>>> > >>>> The standard-C compliant pattern has about 2500 entries: > >>>> > >>>> $ git grep -E "=\s*\{\s*NULL\s*\}"|wc -l > >>>> 137 > >>>> $ git grep -E "=\s*\{\s*0\s*\}"|wc -l > >>>> 2323 > >>>> > >>>> So, let's initialize those structs with: > >>>> = { NULL } > >>>> > >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > >>>> --- > >>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- > >>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- > >>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c > >>>> index b538eb0321d8..44c45c687503 100644 > >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > >>>> @@ -75,7 +75,7 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx) > >>>> memset(ctx->ctrls, 0, ctrl_size); > >>>> > >>>> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) { > >>>> - struct v4l2_ctrl_config cfg = { 0 }; > >>>> + struct v4l2_ctrl_config cfg = { NULL }; > >>>> > >>>> cfg.elem_size = cedrus_controls[i].elem_size; > >>>> cfg.id = cedrus_controls[i].id; > >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > >>>> index e40180a33951..4099a42dba2d 100644 > >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > >>>> @@ -26,7 +26,7 @@ void cedrus_device_run(void *priv) > >>>> { > >>>> struct cedrus_ctx *ctx = priv; > >>>> struct cedrus_dev *dev = ctx->dev; > >>>> - struct cedrus_run run = { 0 }; > >>>> + struct cedrus_run run = { NULL }; > >>>> struct media_request *src_req; > >>>> unsigned long flags; > >>>> > >>>> > >>> > >> > >> > >> > >> Thanks, > >> Mauro > >> > > Thanks, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: cedrus: don't initialize pointers with zero 2018-12-07 11:31 ` Mauro Carvalho Chehab 2018-12-07 11:37 ` Hans Verkuil @ 2018-12-07 13:22 ` Dan Carpenter 1 sibling, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2018-12-07 13:22 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Hans Verkuil, devel, Maxime Ripard, Greg Kroah-Hartman, Mauro Carvalho Chehab, Paul Kocialkowski, Chen-Yu Tsai, linux-arm-kernel, Linux Media Mailing List On Fri, Dec 07, 2018 at 09:31:06AM -0200, Mauro Carvalho Chehab wrote: > Em Fri, 7 Dec 2018 12:14:50 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote: > > > A common mistake is to assume that initializing a var with: > > > struct foo f = { 0 }; > > > > > > Would initialize a zeroed struct. Actually, what this does is > > > to initialize the first element of the struct to zero. > > > > > > According to C99 Standard 6.7.8.21: > > > > > > "If there are fewer initializers in a brace-enclosed > > > list than there are elements or members of an aggregate, > > > or fewer characters in a string literal used to initialize > > > an array of known size than there are elements in the array, > > > the remainder of the aggregate shall be initialized implicitly > > > the same as objects that have static storage duration." > > > > > > So, in practice, it could zero the entire struct, but, if the > > > first element is not an integer, it will produce warnings: > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49: warning: Using plain integer as NULL pointer > > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35: warning: Using plain integer as NULL pointer > > > > > > A proper way to initialize it with gcc is to use: > > > > > > struct foo f = { }; > > > > > > But that seems to be a gcc extension. So, I decided to check upstream > > > > No, this is not a gcc extension. It's part of the latest C standard. > > Sure? Where the C standard spec states that? I've been seeking for > such info for a while, as '= {}' is also my personal preference. > > I tried to build the Kernel with clang, just to be sure that this > won't cause issues with the clang support My test says that clang works with {}. I support this in Smatch as well. regards, dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-07 13:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-07 10:56 [PATCH] media: cedrus: don't initialize pointers with zero Mauro Carvalho Chehab 2018-12-07 11:14 ` Hans Verkuil 2018-12-07 11:31 ` Mauro Carvalho Chehab 2018-12-07 11:37 ` Hans Verkuil 2018-12-07 12:27 ` Ian Arkver 2018-12-07 12:58 ` Mauro Carvalho Chehab 2018-12-07 13:22 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).