| Message ID | 20200415204858.2448-6-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: Refactor function rproc_alloc() | expand |
On 4/15/20 3:48 PM, Mathieu Poirier wrote: > Improve the readability of function rproc_alloc_firmware() by using > a non-negated condition. > > Suggested-by: Alex Elder <elder@linaro.org> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> If it were me, I'd move the comment above the if statement and perhaps reword it a little bit to describe what's happening. But no matter, this looks good. Reviewed-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index ebaff496ef81..0bfa6998705d 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc, > { > const char *p; > > - if (!firmware) > + if (firmware) > + p = kstrdup_const(firmware, GFP_KERNEL); > + else > /* > * If the caller didn't pass in a firmware name then > * construct a default name. > */ > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); > - else > - p = kstrdup_const(firmware, GFP_KERNEL); > > if (!p) > return -ENOMEM; >
… > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc, > { > const char *p; > > - if (!firmware) > + if (firmware) > + p = kstrdup_const(firmware, GFP_KERNEL); > + else > /* > * If the caller didn't pass in a firmware name then > * construct a default name. > */ > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); > - else > - p = kstrdup_const(firmware, GFP_KERNEL); Can the use of the conditional operator make sense at such source code places? p = firmware ? kstrdup_const(…) : kasprintf(…); Regards, Markus
Hi Markus, On 4/16/20 1:26 AM, Markus Elfring wrote: > … >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc, >> { >> const char *p; >> >> - if (!firmware) >> + if (firmware) >> + p = kstrdup_const(firmware, GFP_KERNEL); >> + else >> /* >> * If the caller didn't pass in a firmware name then >> * construct a default name. >> */ >> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); >> - else >> - p = kstrdup_const(firmware, GFP_KERNEL); > > Can the use of the conditional operator make sense at such source code places? > > p = firmware ? kstrdup_const(…) : kasprintf(…); For simple assignments, I too prefer the ternary operator, but in this case, I think it is better to leave the current code as is. regards Suman
>> p = firmware ? kstrdup_const(…) : kasprintf(…); > > For simple assignments, I too prefer the ternary operator, Thanks for your feedback. > but in this case, I think it is better to leave the current code as is. Would you like to consider the use of the function “kvasprintf_const” according to your review comment for the update step “[PATCH v2 4/7] remoteproc: Use kstrdup_const() rather than kstrup()”? Regards, Markus
On 4/17/20 10:48 AM, Markus Elfring wrote: >>> p = firmware ? kstrdup_const(…) : kasprintf(…); >> >> For simple assignments, I too prefer the ternary operator, > > Thanks for your feedback. > > >> but in this case, I think it is better to leave the current code as is. > > Would you like to consider the use of the function “kvasprintf_const” > according to your review comment for the update step “[PATCH v2 4/7] remoteproc: > Use kstrdup_const() rather than kstrup()”? This patch is just swapping the condition order, so will automatically be adjusted for any changes in patch 4 during the rebase. regards Suman
Hi Markus, On Fri, Apr 17, 2020 at 05:48:49PM +0200, Markus Elfring wrote: > >> p = firmware ? kstrdup_const(…) : kasprintf(…); > > > > For simple assignments, I too prefer the ternary operator, > > Thanks for your feedback. > > > > but in this case, I think it is better to leave the current code as is. > > Would you like to consider the use of the function “kvasprintf_const” > according to your review comment for the update step “[PATCH v2 4/7] remoteproc: > Use kstrdup_const() rather than kstrup()”? > Looking at the implementation of kvasprintf_const(), using it here wouldn't give us anything. Indeed allocation of duplicate memory is avoided only if the string is pre determined of if it is exactly %s, which isn't the case here. Otherwise things default to kvasprintf(). Thanks, Mathieu > Regards, > Markus
On Fri, Apr 17, 2020 at 08:39:39AM -0500, Suman Anna wrote: > Hi Markus, > > On 4/16/20 1:26 AM, Markus Elfring wrote: > > … > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc, > > > { > > > const char *p; > > > > > > - if (!firmware) > > > + if (firmware) > > > + p = kstrdup_const(firmware, GFP_KERNEL); > > > + else > > > /* > > > * If the caller didn't pass in a firmware name then > > > * construct a default name. > > > */ > > > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); > > > - else > > > - p = kstrdup_const(firmware, GFP_KERNEL); > > > > Can the use of the conditional operator make sense at such source code places? > > > > p = firmware ? kstrdup_const(…) : kasprintf(…); > > For simple assignments, I too prefer the ternary operator, but in this case, > I think it is better to leave the current code as is. I agree with Suman, that's why I didn't use the conditional operator. > > regards > Suman
On Wed 15 Apr 14:23 PDT 2020, Alex Elder wrote: > On 4/15/20 3:48 PM, Mathieu Poirier wrote: > > Improve the readability of function rproc_alloc_firmware() by using > > a non-negated condition. > > > > Suggested-by: Alex Elder <elder@linaro.org> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > If it were me, I'd move the comment above the if statement and > perhaps reword it a little bit to describe what's happening. > But no matter, this looks good. > This would also avoid the fact that we have a multiline block without braces (which isn't needed, but looks odd to me). So that sounds like a good idea. Regards, Bjorn > Reviewed-by: Alex Elder <elder@linaro.org> > > > --- > > drivers/remoteproc/remoteproc_core.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index ebaff496ef81..0bfa6998705d 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc, > > { > > const char *p; > > > > - if (!firmware) > > + if (firmware) > > + p = kstrdup_const(firmware, GFP_KERNEL); > > + else > > /* > > * If the caller didn't pass in a firmware name then > > * construct a default name. > > */ > > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); > > - else > > - p = kstrdup_const(firmware, GFP_KERNEL); > > > > if (!p) > > return -ENOMEM; > > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index ebaff496ef81..0bfa6998705d 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1984,14 +1984,14 @@ static int rproc_alloc_firmware(struct rproc *rproc, { const char *p; - if (!firmware) + if (firmware) + p = kstrdup_const(firmware, GFP_KERNEL); + else /* * If the caller didn't pass in a firmware name then * construct a default name. */ p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); - else - p = kstrdup_const(firmware, GFP_KERNEL); if (!p) return -ENOMEM;
Improve the readability of function rproc_alloc_firmware() by using a non-negated condition. Suggested-by: Alex Elder <elder@linaro.org> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)