| Message ID | 20250319152126.3472290-4-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanups around returns | expand |
Is this official coding style? I'm not a big fan of having return statements in the middle of functions, I generally only put them at the beginning or the end. -corey On Wed, Mar 19, 2025 at 10:26 AM Markus Armbruster <armbru@redhat.com> wrote: > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/ipmi/ipmi_bmc_extern.c | 4 +--- > hw/ipmi/ipmi_bmc_sim.c | 7 ++----- > hw/ipmi/ipmi_bt.c | 7 +++---- > hw/ipmi/ipmi_kcs.c | 3 +-- > 4 files changed, 7 insertions(+), 14 deletions(-) > > diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c > index 73b249fb60..ab9b66274d 100644 > --- a/hw/ipmi/ipmi_bmc_extern.c > +++ b/hw/ipmi/ipmi_bmc_extern.c > @@ -213,7 +213,7 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b, > rsp[2] = err; > ibe->waiting_rsp = false; > k->handle_rsp(s, msg_id, rsp, 3); > - goto out; > + return; > } > > addchar(ibe, msg_id); > @@ -228,8 +228,6 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b, > > /* Start the transmit */ > continue_send(ibe); > - > - out: > } > > static void handle_hw_op(IPMIBmcExtern *ibe, unsigned char hw_op) > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > index faec6fefb3..f4336946ce 100644 > --- a/hw/ipmi/ipmi_bmc_sim.c > +++ b/hw/ipmi/ipmi_bmc_sim.c > @@ -463,13 +463,12 @@ void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log) > } > > if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) { > - goto out; > + return; > } > > memcpy(ibs->evtbuf, evt, 16); > ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; > k->set_atn(s, 1, attn_irq_enabled(ibs)); > - out: > } > static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert, > uint8_t evd1, uint8_t evd2, uint8_t evd3) > @@ -979,7 +978,7 @@ static void get_msg(IPMIBmcSim *ibs, > > if (QTAILQ_EMPTY(&ibs->rcvbufs)) { > rsp_buffer_set_error(rsp, 0x80); /* Queue empty */ > - goto out; > + return; > } > rsp_buffer_push(rsp, 0); /* Channel 0 */ > msg = QTAILQ_FIRST(&ibs->rcvbufs); > @@ -994,8 +993,6 @@ static void get_msg(IPMIBmcSim *ibs, > ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE; > k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); > } > - > -out: > } > > static unsigned char > diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c > index 3ef1f435e7..f769cfa243 100644 > --- a/hw/ipmi/ipmi_bt.c > +++ b/hw/ipmi/ipmi_bt.c > @@ -98,14 +98,14 @@ static void ipmi_bt_handle_event(IPMIInterface *ii) > IPMIBT *ib = iic->get_backend_data(ii); > > if (ib->inlen < 4) { > - goto out; > + return; > } > /* Note that overruns are handled by handle_command */ > if (ib->inmsg[0] != (ib->inlen - 1)) { > /* Length mismatch, just ignore. */ > IPMI_BT_SET_BBUSY(ib->control_reg, 1); > ib->inlen = 0; > - goto out; > + return; > } > if ((ib->inmsg[1] == (IPMI_NETFN_APP << 2)) && > (ib->inmsg[3] == IPMI_CMD_GET_BT_INTF_CAP)) { > @@ -136,7 +136,7 @@ static void ipmi_bt_handle_event(IPMIInterface *ii) > IPMI_BT_SET_B2H_IRQ(ib->mask_reg, 1); > ipmi_bt_raise_irq(ib); > } > - goto out; > + return; > } > ib->waiting_seq = ib->inmsg[2]; > ib->inmsg[2] = ib->inmsg[1]; > @@ -145,7 +145,6 @@ static void ipmi_bt_handle_event(IPMIInterface *ii) > bk->handle_command(ib->bmc, ib->inmsg + 2, ib->inlen - 2, > sizeof(ib->inmsg), ib->waiting_rsp); > } > - out: > } > > static void ipmi_bt_handle_rsp(IPMIInterface *ii, uint8_t msg_id, > diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c > index f4f1523d6b..5bfc34676f 100644 > --- a/hw/ipmi/ipmi_kcs.c > +++ b/hw/ipmi/ipmi_kcs.c > @@ -168,7 +168,7 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii) > ik->outpos = 0; > bk->handle_command(ik->bmc, ik->inmsg, ik->inlen, sizeof(ik->inmsg), > ik->waiting_rsp); > - goto out_noibf; > + return; > } else if (ik->cmd_reg == IPMI_KCS_WRITE_END_CMD) { > ik->cmd_reg = -1; > ik->write_end = 1; > @@ -197,7 +197,6 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii) > ik->cmd_reg = -1; > ik->data_in_reg = -1; > IPMI_KCS_SET_IBF(ik->status_reg, 0); > - out_noibf: > } > > static void ipmi_kcs_handle_rsp(IPMIInterface *ii, uint8_t msg_id, > -- > 2.48.1 > >
Corey Minyard <corey@minyard.net> writes: > Is this official coding style? I'm not a big fan of having return > statements in the middle of functions, I generally only put them at > the beginning or the end. There's nothing in docs/devel/style.rst. I count more than 42,000 return statements with indentation > 4. These are either within some block, or incorrectly indented. I'd bet my own money that it's the former for pretty much all of them. I count less than 130 labels right before a return statement at end of a function. Based on that, I'd say return in the middle of function is overwhelmingly common in our code.
On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > Corey Minyard <corey@minyard.net> writes: > > > Is this official coding style? I'm not a big fan of having return > > statements in the middle of functions, I generally only put them at > > the beginning or the end. > > There's nothing in docs/devel/style.rst. > > I count more than 42,000 return statements with indentation > 4. These > are either within some block, or incorrectly indented. I'd bet my own > money that it's the former for pretty much all of them. > > I count less than 130 labels right before a return statement at end of a > function. > > Based on that, I'd say return in the middle of function is > overwhelmingly common in our code. > Ok. It's not a huge deal to me. I think it's more dangerous to have returns in the middle; they are easy to miss and an "out:" at the end make it more clear there are returns in the middle. But that's just my opinion. To make wholesale changes like this I would prefer it be in the style guide. But, I don't want to start a holy war, either. Sigh. I mean, just a "return;" at the end of a function, yes, that's a no-brainer, get rid of it. But that's not what the ones in the IPMI device are. Thanks, -corey
Corey Minyard <corey@minyard.net> writes: > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: >> Corey Minyard <corey@minyard.net> writes: >> >> > Is this official coding style? I'm not a big fan of having return >> > statements in the middle of functions, I generally only put them at >> > the beginning or the end. >> >> There's nothing in docs/devel/style.rst. >> >> I count more than 42,000 return statements with indentation > 4. These >> are either within some block, or incorrectly indented. I'd bet my own >> money that it's the former for pretty much all of them. >> >> I count less than 130 labels right before a return statement at end of a >> function. >> >> Based on that, I'd say return in the middle of function is >> overwhelmingly common in our code. >> > > Ok. It's not a huge deal to me. I think it's more dangerous to > have returns in the middle; they are easy to miss and an "out:" at the > end make it more clear there are returns in the middle. But that's > just my opinion. To make wholesale changes like this I would prefer > it be in the style guide. But, I don't want to start a holy war, > either. Sigh. > > I mean, just a "return;" at the end of a function, yes, that's a > no-brainer, get rid of it. But that's not what the ones in the IPMI > device are. Well, you're the maintainer there. If you'd like me to drop the five cases where return is directly after a label (all in hw/ipmi), I can do that for the low, low price of a "yes, please!"
Markus Armbruster <armbru@redhat.com> writes: > Corey Minyard <corey@minyard.net> writes: > >> Is this official coding style? I'm not a big fan of having return >> statements in the middle of functions, I generally only put them at >> the beginning or the end. > > There's nothing in docs/devel/style.rst. > > I count more than 42,000 return statements with indentation > 4. These > are either within some block, or incorrectly indented. I'd bet my own > money that it's the former for pretty much all of them. > > I count less than 130 labels right before a return statement at end of a > function. > > Based on that, I'd say return in the middle of function is > overwhelmingly common in our code. And with autofree variables it saves on clumsy goto and cleanup handlers.
On Wed, Mar 19, 2025 at 08:49:01PM +0100, Markus Armbruster wrote: > Corey Minyard <corey@minyard.net> writes: > > > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > >> Corey Minyard <corey@minyard.net> writes: > >> > >> > Is this official coding style? I'm not a big fan of having return > >> > statements in the middle of functions, I generally only put them at > >> > the beginning or the end. > >> > >> There's nothing in docs/devel/style.rst. > >> > >> I count more than 42,000 return statements with indentation > 4. These > >> are either within some block, or incorrectly indented. I'd bet my own > >> money that it's the former for pretty much all of them. > >> > >> I count less than 130 labels right before a return statement at end of a > >> function. > >> > >> Based on that, I'd say return in the middle of function is > >> overwhelmingly common in our code. > >> > > > > Ok. It's not a huge deal to me. I think it's more dangerous to > > have returns in the middle; they are easy to miss and an "out:" at the > > end make it more clear there are returns in the middle. But that's > > just my opinion. To make wholesale changes like this I would prefer > > it be in the style guide. But, I don't want to start a holy war, > > either. Sigh. > > > > I mean, just a "return;" at the end of a function, yes, that's a > > no-brainer, get rid of it. But that's not what the ones in the IPMI > > device are. > > Well, you're the maintainer there. If you'd like me to drop the five > cases where return is directly after a label (all in hw/ipmi), I can do > that for the low, low price of a "yes, please!" > No, I'm fine, I woudl just like it in the style guide. Signed-off-by: Corey Minyard <cminyard@mvista.com>
On Wed, Mar 19, 2025 at 03:51:45PM -0500, Corey Minyard wrote: > On Wed, Mar 19, 2025 at 08:49:01PM +0100, Markus Armbruster wrote: > > Corey Minyard <corey@minyard.net> writes: > > > > > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > > >> Corey Minyard <corey@minyard.net> writes: > > >> > > >> > Is this official coding style? I'm not a big fan of having return > > >> > statements in the middle of functions, I generally only put them at > > >> > the beginning or the end. > > >> > > >> There's nothing in docs/devel/style.rst. > > >> > > >> I count more than 42,000 return statements with indentation > 4. These > > >> are either within some block, or incorrectly indented. I'd bet my own > > >> money that it's the former for pretty much all of them. > > >> > > >> I count less than 130 labels right before a return statement at end of a > > >> function. > > >> > > >> Based on that, I'd say return in the middle of function is > > >> overwhelmingly common in our code. > > >> > > > > > > Ok. It's not a huge deal to me. I think it's more dangerous to > > > have returns in the middle; they are easy to miss and an "out:" at the > > > end make it more clear there are returns in the middle. But that's > > > just my opinion. To make wholesale changes like this I would prefer > > > it be in the style guide. But, I don't want to start a holy war, > > > either. Sigh. > > > > > > I mean, just a "return;" at the end of a function, yes, that's a > > > no-brainer, get rid of it. But that's not what the ones in the IPMI > > > device are. > > > > Well, you're the maintainer there. If you'd like me to drop the five > > cases where return is directly after a label (all in hw/ipmi), I can do > > that for the low, low price of a "yes, please!" > > > > No, I'm fine, I woudl just like it in the style guide. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> I mean: Acked-by: Corey Minyard <cminyard@mvista.com>
On Wed, Mar 19, 2025 at 02:42:21PM -0500, Corey Minyard wrote: > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > > Corey Minyard <corey@minyard.net> writes: > > > > > Is this official coding style? I'm not a big fan of having return > > > statements in the middle of functions, I generally only put them at > > > the beginning or the end. > > > > There's nothing in docs/devel/style.rst. > > > > I count more than 42,000 return statements with indentation > 4. These > > are either within some block, or incorrectly indented. I'd bet my own > > money that it's the former for pretty much all of them. > > > > I count less than 130 labels right before a return statement at end of a > > function. > > > > Based on that, I'd say return in the middle of function is > > overwhelmingly common in our code. > > > > Ok. It's not a huge deal to me. I think it's more dangerous to > have returns in the middle; they are easy to miss and an "out:" at the > end make it more clear there are returns in the middle. But that's > just my opinion. To make wholesale changes like this I would prefer > it be in the style guide. But, I don't want to start a holy war, > either. Sigh. In traditional C, I would agree with you that mid-function 'return's are often a bad idea, because they complicate free'ing of memory and tend to actively encourage memory/resource leaks. With our adoption of g_auto/g_autofree, that problem has been eliminated across a decently large subset of code. This swings the balance so that having mid-function 'return's often (but not always) results in shorter & easier to understand code, with few leak possiblities, provided g_auto/autofree is sufficient to deal with all cleanup needs. There will still be cases where it makes more sense to use 'goto' for cleanup, since g_auto/autofree is sufficient in all scenarios. Thus I don't think we should have a rule that strictly dictates either way. Better to leave it upto author's judgement call as to which approach results in clearer code for each particular function. I would still encourage maximising use of 'g_auto/autofree' where practical. With regards, Daniel
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c index 73b249fb60..ab9b66274d 100644 --- a/hw/ipmi/ipmi_bmc_extern.c +++ b/hw/ipmi/ipmi_bmc_extern.c @@ -213,7 +213,7 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b, rsp[2] = err; ibe->waiting_rsp = false; k->handle_rsp(s, msg_id, rsp, 3); - goto out; + return; } addchar(ibe, msg_id); @@ -228,8 +228,6 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b, /* Start the transmit */ continue_send(ibe); - - out: } static void handle_hw_op(IPMIBmcExtern *ibe, unsigned char hw_op) diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c index faec6fefb3..f4336946ce 100644 --- a/hw/ipmi/ipmi_bmc_sim.c +++ b/hw/ipmi/ipmi_bmc_sim.c @@ -463,13 +463,12 @@ void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log) } if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) { - goto out; + return; } memcpy(ibs->evtbuf, evt, 16); ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL; k->set_atn(s, 1, attn_irq_enabled(ibs)); - out: } static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert, uint8_t evd1, uint8_t evd2, uint8_t evd3) @@ -979,7 +978,7 @@ static void get_msg(IPMIBmcSim *ibs, if (QTAILQ_EMPTY(&ibs->rcvbufs)) { rsp_buffer_set_error(rsp, 0x80); /* Queue empty */ - goto out; + return; } rsp_buffer_push(rsp, 0); /* Channel 0 */ msg = QTAILQ_FIRST(&ibs->rcvbufs); @@ -994,8 +993,6 @@ static void get_msg(IPMIBmcSim *ibs, ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE; k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs)); } - -out: } static unsigned char diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c index 3ef1f435e7..f769cfa243 100644 --- a/hw/ipmi/ipmi_bt.c +++ b/hw/ipmi/ipmi_bt.c @@ -98,14 +98,14 @@ static void ipmi_bt_handle_event(IPMIInterface *ii) IPMIBT *ib = iic->get_backend_data(ii); if (ib->inlen < 4) { - goto out; + return; } /* Note that overruns are handled by handle_command */ if (ib->inmsg[0] != (ib->inlen - 1)) { /* Length mismatch, just ignore. */ IPMI_BT_SET_BBUSY(ib->control_reg, 1); ib->inlen = 0; - goto out; + return; } if ((ib->inmsg[1] == (IPMI_NETFN_APP << 2)) && (ib->inmsg[3] == IPMI_CMD_GET_BT_INTF_CAP)) { @@ -136,7 +136,7 @@ static void ipmi_bt_handle_event(IPMIInterface *ii) IPMI_BT_SET_B2H_IRQ(ib->mask_reg, 1); ipmi_bt_raise_irq(ib); } - goto out; + return; } ib->waiting_seq = ib->inmsg[2]; ib->inmsg[2] = ib->inmsg[1]; @@ -145,7 +145,6 @@ static void ipmi_bt_handle_event(IPMIInterface *ii) bk->handle_command(ib->bmc, ib->inmsg + 2, ib->inlen - 2, sizeof(ib->inmsg), ib->waiting_rsp); } - out: } static void ipmi_bt_handle_rsp(IPMIInterface *ii, uint8_t msg_id, diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c index f4f1523d6b..5bfc34676f 100644 --- a/hw/ipmi/ipmi_kcs.c +++ b/hw/ipmi/ipmi_kcs.c @@ -168,7 +168,7 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii) ik->outpos = 0; bk->handle_command(ik->bmc, ik->inmsg, ik->inlen, sizeof(ik->inmsg), ik->waiting_rsp); - goto out_noibf; + return; } else if (ik->cmd_reg == IPMI_KCS_WRITE_END_CMD) { ik->cmd_reg = -1; ik->write_end = 1; @@ -197,7 +197,6 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii) ik->cmd_reg = -1; ik->data_in_reg = -1; IPMI_KCS_SET_IBF(ik->status_reg, 0); - out_noibf: } static void ipmi_kcs_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/ipmi/ipmi_bmc_extern.c | 4 +--- hw/ipmi/ipmi_bmc_sim.c | 7 ++----- hw/ipmi/ipmi_bt.c | 7 +++---- hw/ipmi/ipmi_kcs.c | 3 +-- 4 files changed, 7 insertions(+), 14 deletions(-)