* [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. @ 2011-05-23 11:07 Hans Petter Selasky 2011-05-23 18:22 ` Guennadi Liakhovetski 0 siblings, 1 reply; 5+ messages in thread From: Hans Petter Selasky @ 2011-05-23 11:07 UTC (permalink / raw) To: linux-media; +Cc: Mauro Carvalho Chehab [-- Attachment #1: Type: text/plain, Size: 6 bytes --] --HPS [-- Attachment #2: dvb-usb-0005.patch --] [-- Type: text/x-patch, Size: 1237 bytes --] From 94b88b92763f9309018ba04c200a8842ce1ff0ed Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky <hselasky@c2i.net> Date: Mon, 23 May 2011 13:07:08 +0200 Subject: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. Signed-off-by: Hans Petter Selasky <hselasky@c2i.net> --- drivers/media/video/sr030pc30.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c index c901721..6cc64c9 100644 --- a/drivers/media/video/sr030pc30.c +++ b/drivers/media/video/sr030pc30.c @@ -726,8 +726,10 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) const struct sr030pc30_platform_data *pdata = info->pdata; int ret; - if (WARN(pdata == NULL, "No platform data!\n")) + if (pdata == NULL) { + WARN(1, "No platform data!\n"); return -ENOMEM; + } /* * Put sensor into power sleep mode before switching off @@ -746,6 +748,7 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) if (on) { ret = sr030pc30_base_config(sd); } else { + ret = 0; info->curr_win = NULL; info->curr_fmt = NULL; } -- 1.7.1.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. 2011-05-23 11:07 [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case Hans Petter Selasky @ 2011-05-23 18:22 ` Guennadi Liakhovetski 2011-05-23 19:04 ` Hans Petter Selasky 0 siblings, 1 reply; 5+ messages in thread From: Guennadi Liakhovetski @ 2011-05-23 18:22 UTC (permalink / raw) To: Hans Petter Selasky; +Cc: linux-media, Mauro Carvalho Chehab Please, inline patches. Otherwise, this is what one gets, when replying. On Mon, 23 May 2011, Hans Petter Selasky wrote: > --HPS > In any case, just throwing in my 2 cents - no idea how not using the return value of WARN() makes code more readable. On the contrary, using it is a standard practice. This patch doesn't seem like an improvement to me. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. 2011-05-23 18:22 ` Guennadi Liakhovetski @ 2011-05-23 19:04 ` Hans Petter Selasky 2011-05-26 0:03 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 5+ messages in thread From: Hans Petter Selasky @ 2011-05-23 19:04 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-media, Mauro Carvalho Chehab On Monday 23 May 2011 20:22:02 Guennadi Liakhovetski wrote: > Please, inline patches. Otherwise, this is what one gets, when replying. > > On Mon, 23 May 2011, Hans Petter Selasky wrote: > > --HPS > > In any case, just throwing in my 2 cents - no idea how not using the > return value of WARN() makes code more readable. On the contrary, using it > is a standard practice. This patch doesn't seem like an improvement to me. There is no strong reason for the WARN() part, you may ignore that, but the ret = 0, part is still valid. Should I generate a new patch or can you handle this? --HPS ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. 2011-05-23 19:04 ` Hans Petter Selasky @ 2011-05-26 0:03 ` Mauro Carvalho Chehab 2011-05-26 6:21 ` Guennadi Liakhovetski 0 siblings, 1 reply; 5+ messages in thread From: Mauro Carvalho Chehab @ 2011-05-26 0:03 UTC (permalink / raw) To: Hans Petter Selasky; +Cc: Guennadi Liakhovetski, linux-media Em 23-05-2011 16:04, Hans Petter Selasky escreveu: > On Monday 23 May 2011 20:22:02 Guennadi Liakhovetski wrote: >> Please, inline patches. Otherwise, this is what one gets, when replying. >> >> On Mon, 23 May 2011, Hans Petter Selasky wrote: >>> --HPS >> >> In any case, just throwing in my 2 cents - no idea how not using the >> return value of WARN() makes code more readable. On the contrary, using it >> is a standard practice. This patch doesn't seem like an improvement to me. > > There is no strong reason for the WARN() part, you may ignore that, but the > ret = 0, part is still valid. Should I generate a new patch or can you handle > this? Em 23-05-2011 08:07, Hans Petter Selasky escreveu: > --HPS > > > dvb-usb-0005.patch > > > From 94b88b92763f9309018ba04c200a8842ce1ff0ed Mon Sep 17 00:00:00 2001 > From: Hans Petter Selasky <hselasky@c2i.net> > Date: Mon, 23 May 2011 13:07:08 +0200 > Subject: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. > > Signed-off-by: Hans Petter Selasky <hselasky@c2i.net> > --- > drivers/media/video/sr030pc30.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c > index c901721..6cc64c9 100644 > --- a/drivers/media/video/sr030pc30.c > +++ b/drivers/media/video/sr030pc30.c > @@ -726,8 +726,10 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) > const struct sr030pc30_platform_data *pdata = info->pdata; > int ret; > > - if (WARN(pdata == NULL, "No platform data!\n")) > + if (pdata == NULL) { > + WARN(1, "No platform data!\n"); > return -ENOMEM; > + } > > /* > * Put sensor into power sleep mode before switching off > @@ -746,6 +748,7 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) > if (on) { > ret = sr030pc30_base_config(sd); > } else { > + ret = 0; > info->curr_win = NULL; > info->curr_fmt = NULL; > } > -- 1.7.1.1 IMHO, both hunks make sense, as, on the first hunk, it is returning an error condition. Yet, -ENOMEM seems to be the wrong return code. -EINVAL is probably more appropriate. However, the patch is badly described. It is not about making the code cleaner, but about avoiding to run s_power if no platform data is found, and to avoid having ret undefined. Eventually, it should be broken into two different patches, as they fix different things. Please, when sending us patches, provide a proper description with "what" information at the first line, and why and how at the patch descriptions. Please, also avoid to have any line bigger than 74 characters, otherwise they'll look weird when seeing the patch history. Thanks, Mauro. information a ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. 2011-05-26 0:03 ` Mauro Carvalho Chehab @ 2011-05-26 6:21 ` Guennadi Liakhovetski 0 siblings, 0 replies; 5+ messages in thread From: Guennadi Liakhovetski @ 2011-05-26 6:21 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Hans Petter Selasky, linux-media On Wed, 25 May 2011, Mauro Carvalho Chehab wrote: > Em 23-05-2011 16:04, Hans Petter Selasky escreveu: > > On Monday 23 May 2011 20:22:02 Guennadi Liakhovetski wrote: > >> Please, inline patches. Otherwise, this is what one gets, when replying. > >> > >> On Mon, 23 May 2011, Hans Petter Selasky wrote: > >>> --HPS > >> > >> In any case, just throwing in my 2 cents - no idea how not using the > >> return value of WARN() makes code more readable. On the contrary, using it > >> is a standard practice. This patch doesn't seem like an improvement to me. > > > > There is no strong reason for the WARN() part, you may ignore that, but the > > ret = 0, part is still valid. Should I generate a new patch or can you handle > > this? > Em 23-05-2011 08:07, Hans Petter Selasky escreveu: > > --HPS > > > > > > dvb-usb-0005.patch > > > > > > From 94b88b92763f9309018ba04c200a8842ce1ff0ed Mon Sep 17 00:00:00 2001 > > From: Hans Petter Selasky <hselasky@c2i.net> > > Date: Mon, 23 May 2011 13:07:08 +0200 > > Subject: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. > > > > Signed-off-by: Hans Petter Selasky <hselasky@c2i.net> > > --- > > drivers/media/video/sr030pc30.c | 5 ++++- > > 1 files changed, 4 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c > > index c901721..6cc64c9 100644 > > --- a/drivers/media/video/sr030pc30.c > > +++ b/drivers/media/video/sr030pc30.c > > @@ -726,8 +726,10 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) > > const struct sr030pc30_platform_data *pdata = info->pdata; > > int ret; > > > > - if (WARN(pdata == NULL, "No platform data!\n")) > > + if (pdata == NULL) { > > + WARN(1, "No platform data!\n"); > > return -ENOMEM; > > + } > > > > /* > > * Put sensor into power sleep mode before switching off > > @@ -746,6 +748,7 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) > > if (on) { > > ret = sr030pc30_base_config(sd); > > } else { > > + ret = 0; > > info->curr_win = NULL; > > info->curr_fmt = NULL; > > } > > -- 1.7.1.1 > > IMHO, both hunks make sense, as, on the first hunk, it is returning an error condition. > Yet, -ENOMEM seems to be the wrong return code. -EINVAL is probably more appropriate. Sorry, Mauro, don't understand. AFAICS, the first hunk makes _no_ functional difference, it only obfuscates a perfectly valid use of the WARN() macro. Thanks Guennadi > > However, the patch is badly described. It is not about making the code cleaner, but > about avoiding to run s_power if no platform data is found, and to avoid having > ret undefined. Eventually, it should be broken into two different patches, as they > fix different things. > > Please, when sending us patches, provide a proper description with "what" information > at the first line, and why and how at the patch descriptions. Please, also avoid to > have any line bigger than 74 characters, otherwise they'll look weird when seeing the > patch history. > > Thanks, > Mauro. > information a > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-05-26 6:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-05-23 11:07 [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case Hans Petter Selasky 2011-05-23 18:22 ` Guennadi Liakhovetski 2011-05-23 19:04 ` Hans Petter Selasky 2011-05-26 0:03 ` Mauro Carvalho Chehab 2011-05-26 6:21 ` Guennadi Liakhovetski
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.