Re: [PATCH 11/29] bisect: libify `bisect_next_all`

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 21, 2020, 07:15 UTC
Message-ID: <CAP8UFD3QP3QXe32cdK+q=P+v3dK1Bfws5KgNnm-JfDnPkpBZfw@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2001202325000.46@tvgsbejvaqbjf.bet>

On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> On Mon, 20 Jan 2020, Miriam Rubio wrote:

Show 19 quoted lines

> > Since we want to get rid of git-bisect.sh it would be necessary to
> > convert those exit() calls to return statements so that errors can be
> > reported.
> >
> > Emulate try catch in C by converting `exit(<positive-value>)` to
> > `return <negative-value>`. Follow POSIX conventions to return
> > <negative-value> to indicate error.
> >
> > Turn `exit()` to `return` calls in `bisect_next_all()`.
> >
> > All the functions calling `bisect_next_all()` are already able to
> > handle return values from it.
>
> So this patch brings more magic values that really would like to become
> `enum` values. At this point, we're talking about `bisect_next_all()`
> which is _not_ a file-local (`static`) function. Therefore, we now really
> wade into API territory where an `enum` is no longer just a nice thing,
> but a necessary thing: `bisect_next_all()` is a function that can be
> called from other source files.

I agree that return values could be better documented. About enum though, I am perhaps wrong but I don't think that we use them often for error codes. Do you have examples in the code base where they are used for such a purpose along with regular `error(...)` calls?

> As far as I can see, this patch concludes the "libify" part of the patch
> series (read: it would have been the second patch series I would have
> suggested to split out from an otherwise too-long-to-be-reviewed set of
> patches, if I had been your mentor),

I am ok with saying that "part 1" stops after this patch and that the rest will be "part 2" and will not be resent in the version 2 of the "part 1" patch series, but will rather be resent later, when "part 1" will be in next for example.

> and I'll leave it at that for
> tonight; Hopefully I will find some time to review more of these patches
> tomorrow.

That would be great!

Thanks, Christian.