| Message ID | 5343E571.60407@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > In function, if no additional resources to free before quit, commonly, > need not use additional local variable 'res' to notice about it. So > remove it to simplify code. > Styling wise, there is a school of thought that functions should only have one return statement which is probably the original authors intention. Regards, Peter > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > vl.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/vl.c b/vl.c > index 7505002..377f962 100644 > --- a/vl.c > +++ b/vl.c > @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position) > { > uint32_t counter = 0; > FWBootEntry *i = NULL; > - DeviceState *res = NULL; > > if (!QTAILQ_EMPTY(&fw_boot_order)) { > QTAILQ_FOREACH(i, &fw_boot_order, link) { > if (counter == position) { > - res = i->dev; > - break; > + return i->dev; > } > counter++; > } > } > - return res; > + return NULL; > } > > /* > -- > 1.7.9.5 >
On 04/15/2014 10:13 AM, Peter Crosthwaite wrote: > On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> In function, if no additional resources to free before quit, commonly, >> need not use additional local variable 'res' to notice about it. So >> remove it to simplify code. >> > > Styling wise, there is a school of thought that functions should only > have one return statement which is probably the original authors > intention. > Hmm... maybe. For me, the readers' feeling (let code simple and easy understanding) is more important than "school rules". Welcome other members' styling tastes. > Regards, > Peter > >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> vl.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 7505002..377f962 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position) >> { >> uint32_t counter = 0; >> FWBootEntry *i = NULL; >> - DeviceState *res = NULL; >> >> if (!QTAILQ_EMPTY(&fw_boot_order)) { >> QTAILQ_FOREACH(i, &fw_boot_order, link) { >> if (counter == position) { >> - res = i->dev; >> - break; >> + return i->dev; >> } >> counter++; >> } >> } >> - return res; >> + return NULL; >> } >> >> /* >> -- >> 1.7.9.5 >> Thanks.
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> In function, if no additional resources to free before quit, commonly, >> need not use additional local variable 'res' to notice about it. So >> remove it to simplify code. >> > > Styling wise, there is a school of thought that functions should only > have one return statement which is probably the original authors > intention. Plausible. But what matters here is whether we think the patch is worthwhile or not. I find Chen's version a bit clearer, but I'm not sure it's worth the churn.
On 04/15/2014 04:43 PM, Markus Armbruster wrote: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> On Tue, Apr 8, 2014 at 10:02 PM, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >>> In function, if no additional resources to free before quit, commonly, >>> need not use additional local variable 'res' to notice about it. So >>> remove it to simplify code. >>> >> >> Styling wise, there is a school of thought that functions should only >> have one return statement which is probably the original authors >> intention. > > Plausible. But what matters here is whether we think the patch is > worthwhile or not. > > I find Chen's version a bit clearer, but I'm not sure it's worth the > churn. > Hmm... after think of, for me, it will be fine to still remain the original state, it is not quit worth to churn. Thanks.
diff --git a/vl.c b/vl.c index 7505002..377f962 100644 --- a/vl.c +++ b/vl.c @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position) { uint32_t counter = 0; FWBootEntry *i = NULL; - DeviceState *res = NULL; if (!QTAILQ_EMPTY(&fw_boot_order)) { QTAILQ_FOREACH(i, &fw_boot_order, link) { if (counter == position) { - res = i->dev; - break; + return i->dev; } counter++; } } - return res; + return NULL; } /*
In function, if no additional resources to free before quit, commonly, need not use additional local variable 'res' to notice about it. So remove it to simplify code. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- vl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)