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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2020, 15:18 UTC
Message-ID: <nycvar.QRO.7.76.6.2001301614410.46@tvgsbejvaqbjf.bet>
In-Reply-To: <CAP8UFD3QP3QXe32cdK+q=P+v3dK1Bfws5KgNnm-JfDnPkpBZfw@mail.gmail.com>

Hi Chris,

On Tue, 21 Jan 2020, Christian Couder wrote:

Show 28 quoted lines

> On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Mon, 20 Jan 2020, Miriam Rubio wrote:
>
> > > 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.

It does not matter how often we actually use them, it matters more whether we _want_ to use them. And in the recent years, we have definitely made more use of enums than before, to allow the compiler to catch mistakes earlier. We even started to convert existing constants to enums, for example.

So I don't think that it is sound advice here to point to the code base.

If you _must_ value the existing practice over a clearly communicated review, then please look no further than at the declaration of `safe_create_leading_directories()`.

> Do you have examples in the code base where they are used for such a
> purpose along with regular `error(...)` calls?

It's all in the code. You don't need me to read it aloud for you.

:-)

Ciao, Dscho