unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Paul Eggert <eggert@cs.ucla.edu>
Cc: 18006@debbugs.gnu.org
Subject: bug#18006: Simplify via set_binary_mode
Date: Sun, 13 Jul 2014 07:20:26 +0300	[thread overview]
Message-ID: <837g3hanzp.fsf@gnu.org> (raw)
In-Reply-To: <53C1A00C.6050906@cs.ucla.edu>

> Date: Sat, 12 Jul 2014 13:52:28 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> Attached is a proposed minor simplification to have Emacs use 
> set_binary_mode more consistently.  I'm filing this as a bug report to 
> give Eli a heads-up, as it affects the Microsoft ports.  This is 
> relative to trunk bzr 117525.

Thanks.

However, I don't understand what is the purpose of this patch.  If we
want to clean up uses of setmode and related issues, I'm all for it
(some of that code is old and unnecessary).  But then the Gnulib
binary-io module is not the answer, or at least not all of it.  Some
of the reasons:

  . in some places, we want all file I/O to be in binary mode; Gnulib
    doesn't support that

  . some of the code needs to switch a file descriptor to binary or
    text mode only when the descriptor is or isn't connected to a
    terminal device; Gnulib is ambivalent about that (it always does
    that for MSDOS, and never for Windows)

  . AFAIK, Gnulib's binary-io also replaces isatty, which is not
    really needed in Emacs (you don't show the patches for lib/ so I
    can only guess)

  . we don't want in Emacs the msvc-nothrow wrappers for library
    functions

Some specific comments about the patch:

> @@ -155,20 +148,12 @@
>  
>        if (un_flag)
>  	{
> -	  char buf[18];
> +	  set_binary_mode (fileno (stdout), O_BINARY);
>  
> -#ifdef DOS_NT
> -#if (__DJGPP__ >= 2) || (defined WINDOWSNT)
> -          if (!isatty (fileno (stdout)))
> -	    setmode (fileno (stdout), O_BINARY);
> -#else
> -	  (stdout)->_flag &= ~_IOTEXT; /* print binary */
> -	  _setmode (fileno (stdout), O_BINARY);
> -#endif
> -#endif

We no longer support DJGPP < 2, so the #else stuff could simply go
away.

> -#ifdef DOS_NT
> -#if (__DJGPP__ >= 2) || (defined WINDOWSNT)
> -          if (!isatty (fileno (fp)))
> -	    setmode (fileno (fp), O_BINARY);
> -#else
> -	  (fp)->_flag &= ~_IOTEXT; /* read binary */
> -	  _setmode (fileno (fp), O_BINARY);
> -#endif
> -#endif

Likewise.

>    /* Don't put CRs in the DOC file.  */
> -#ifdef MSDOS
> -  _fmode = O_BINARY;
> -#if 0  /* Suspicion is that this causes hanging.
> -	  So instead we require people to use -o on MSDOS.  */
> -  (stdout)->_flag &= ~_IOTEXT;
> -  _setmode (fileno (stdout), O_BINARY);
> -#endif
> -  outfile = 0;
> -#endif /* MSDOS */
> -#ifdef WINDOWSNT
> -  _fmode = O_BINARY;
> -  _setmode (fileno (stdout), O_BINARY);
> -#endif /* WINDOWSNT */
> +  set_binary_mode (fileno (stdout), O_BINARY);

This is wrong: setting _fmode affects all I/O, input and output, not
just stdout.  Gnulib's binary-io doesn't have the equivalent
functionality.

> @@ -239,11 +237,8 @@
>    /* Manipulate tty.  */
>    if (hide_char)
>      {
> -      emacs_get_tty (fileno (stdin), &etty);
> -#ifdef WINDOWSNT
> -      if (isatty (fileno (stdin)))
> -	_setmode (fileno (stdin), O_BINARY);
> -#endif
> +      if (emacs_get_tty (fileno (stdin), &etty) == 0)
> +	set_binary_mode (fileno (stdin), O_BINARY);
>        suppress_echo_on_tty (fileno (stdin));

This logic is flawed: if emacs_get_tty failed, then emacs_set_tty
should not be called as well.

>  #endif	/* WINDOWSNT */
> +  return isatty (fd) - 1;

This will not work with Windows isatty, because it doesn't return 1
when fd is connected to a console.

To summarize, if we want to clean up that code, I'm willing to do the
job.  Bug Gnulib's binary-io is not the answer.

Thanks.





  reply	other threads:[~2014-07-13  4:20 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-12 20:52 bug#18006: Simplify via set_binary_mode Paul Eggert
2014-07-13  4:20 ` Eli Zaretskii [this message]
2014-07-13  5:28   ` Paul Eggert
2014-07-13 15:02     ` Eli Zaretskii
2014-07-14  2:11       ` Paul Eggert
2014-07-14 15:21         ` Eli Zaretskii
2014-07-14 20:16           ` Paul Eggert
2014-07-15 14:37             ` Eli Zaretskii
2014-07-15 19:39               ` Paul Eggert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=837g3hanzp.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=18006@debbugs.gnu.org \
    --cc=eggert@cs.ucla.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).