Re: [PATCH v2] git-compat-util: undefine fileno if defined

From: Duy Nguyen <pclouds@gmail.com>
Date: May 8, 2019, 10:09 UTC
Message-ID: <CACsJy8BcyD199L4qGv6-TP-8HD+GS+ZDNN5jspkh5uVaWekkoQ@mail.gmail.com>
In-Reply-To: <xmqqk1f1gzgs.fsf@gitster-ct.c.googlers.com>

On Wed, May 8, 2019 at 2:47 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

>
> Dan McGregor <dkm560@usask.ca> writes:
>
> >>I don't think this is enough. At least fbsd defines this
> >>
> >>#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))
> >>
> >>so one of the two functions will be used depending on __isthreaded
> >>flag. Your forcing to use fileno, ignoring __sfileno, is technically
> >>not correct.
> >>
> >>For the record, at least fbsd also defines feof, ferror, clearerr,
> >>getc and putc in the same way. But at least I don't see how something
> >>like feof(fp++) could cause bad side effects.
> >>
> >>So, how about something like this? A teeny bit longer than your
> >>version, but I think it's easier to control long term.
> >
> > Yes, this looks pretty reasonable to me too.
>
> Sorry for pinging this ancient thread, but while reviewing the
> stalled topics, this one caught my attention.  The very original
> <20190201193004.88736-1-dan.mcgregor@usask.ca> said that the problem
> it wants to solve was that the code that passes (void*) parameter to
> fileno(), fflush() and rewind() misbehaved, as these are all macros
> on your system.
>
> We solved the problem for fileno() being a macro eventually with
> 18a4f6be ("git-compat-util: work around fileno(fp) that is a macro",
> 2019-02-12), but what about the other two?

I don't think the other two were the problem. Even in the mail you pointed to, only fileno() is acknowledged the problem in the commit message.

At least for BSDs fflush() and rewind() are not macros. BSDs optimize a couple functions for the no-pthread case by inlining some short expressions directly [1]. This works well for read-only functions such as fileno(), feof(), ferror().. and pushing the limits a bit with fputc() and fgetc(), but fflush() and rewind() are (I think) just too complicated to do it this way.

[1] https://github.com/freebsd/freebsd/blob/master/include/stdio.h#L451-L519 [2] https://public-inbox.org/git/20190209023621.75255-1-dan.mcgregor@usask.ca/

> Here comes a weather-balloon to see if we should pursue tying this
> loose end.

Let's see if there are still problems with more exotic platforms. I'm reluctant of adding unused compat/ code because to me compat/ is scary. I often don't have the right systems to test whenever I need to make changes in compat/

-- 
Duy