Re: [PATCH] branch: trivial style fix
- From
Jeff King <peff@peff.net>
- Date
- Oct 15, 2018, 19:35 UTC
- Message-ID
- <20181015193525.GA8349@sigill.intra.peff.net>
- In-Reply-To
- <87bm87x5z2.fsf@ls-a.me>
On Sat, Oct 06, 2018 at 08:40:33AM +0800, Tao Qingyun wrote:
Show 23 quoted lines
> >> - for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
> >> + for (i = 0; i < argc; i++) {
> >> char *target = NULL;
> >> int flags = 0;
> >>
> >> + strbuf_reset(&bname);
> >
> > This is not "trivial" nor "style fix", though.
> >
> > It is not "trivial" because it also changes the behaviour. Instead
> > of resetting the strbuf each time after the loop body runs, the new
> > code resets it before the loop body, even for the 0-th iteration
> > where the strbuf hasn't even been used at all. It is not a "style
> > fix" because we do not have a particular reason to avoid using the
> > comma operator to perform two simple expressions with side effects
> > inside a single expression.
> >
> Thank you and Jeff for your explanation. I think I get the point now.
>
> The third part of `for` statement is normally for a step. I think it's
> improve readability even it does nothing in the first iteration.
>
> And, should I drop this part and resend the patch? I'm a newbie :).Sorry for the slow reply. For some reason I do not think your message here made it to the list (but I don't see anything obviously wrong with it).
Anyway, yes, I think it is worth dropping this hunk and re-sending the else-if style fix as a separate patch (you may choose to re-send this hunk as its own patch, too, if you want to argue for its inclusion, but there is no sense in holding the actual style fix hostage).
-Peff