Re: [PATCH 4/4] m4_esyscmd: fdopen() with a text mode explicitly on OS/2

[Gary V. Vaughan]

Hi Eric,

> On Nov 19, 2014, at 3:33 PM, Eric Blake <address@hidden> wrote:
> 
> On 11/18/2014 08:54 PM, KO Myung-Hun wrote:
>> On OS/2 kLIBC, fdopen() creates a stream in a mode of a file
>> descriptor. So specify "t" to open a stream in a text mode explicitly
>> on OS/2.
>> 
>> * src/builtin.c (m4_esyscmd): fdopen() in a text mode on OS/2.
>> ---
>> src/builtin.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/builtin.c b/src/builtin.c
>> index e101838..7a73b36 100644
>> --- a/src/builtin.c
>> +++ b/src/builtin.c
>> @@ -1019,7 +1019,12 @@ m4_esyscmd (struct obstack *obs, int argc, token_data 
>> **argv)
>>       sysval = 127;
>>       return;
>>     }
>> -  pin = fdopen (fd, "r");
>> +#if OS2
>> +# define MODE_TEXT "t"
>> +#else
>> +# define MODE_TEXT ""
>> +#endif
> 
> Eww.  Mid-function #ifdefs are evil.  I strongly prefer that we hoist it
> out of the function.  Also, fdopen("rt") looks awkward, since 't' mode
> is non-standard.  Why do you need text mode?  What is the default if you
> omit 't', and why is binary mode not good enough?  I'm very reluctant to
> see this patch applied as-is without more reasoning and/or more
> refactoring for easier maintenance.

I changed the implementation before pushing.  Assuming the OP confirms that, 
after
my edits, m4 still works correctly on their architecture, it makes sense that we
escalate those edits to the gnulib fdopen module where it can help other 
projects.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)