[RFC] diagnostics.c: For terminals, restrict messages to terminal width?
diff mbox

Message ID CAESRpQAng5wU3iLEWHqEXn7GSa4CbqdpJF3ep0dQt_Qz8suoZQ@mail.gmail.com
State New
Headers show

Commit Message

Manuel López-Ibáñez Dec. 10, 2014, 12:20 p.m. UTC
On 10 December 2014 at 12:10, Dodji Seketeli <dodji@redhat.com> wrote:
>>
>> Note that -fmessage-length= applies to the error message (wraps) _and_
>> the caret diagnostic (truncates) while the COLUMNS variable _only_
>> applies to the caret diagnostic. (BTW: The documentation currently
>> does not mention COLUMNS.)
>
> I guess we should adjust the documentation to mention COLUMNS.
>
> Manuel, was there a particular reason to avoid mentioning the COLUMNS
> environment variable in the documentation?

Not that I remember. Perhaps the documentation should say something
like: "The line is truncated to fit into n characters only if the
option -fmessage-length=n is given, or if the output is a TTY and the
COLUMNS environment variable is set."

>> (b) Should ioctl be always used or only for Fortran?
>
> I'd go for using it in the common diagnostics framework, unless there is
> a sound motivated reason.  Manuel, do you remember why we didn't query the
> TIOCGWINSZ ioctl property to get the terminal size when that capability
> was available?

I was not aware this possibility even existed.

>> Comments?

Note that Fortran has this:

#ifdef GWINSZ_IN_SYS_IOCTL
# include <sys/ioctl.h>
#endif

Not sure if this is needed for diagnostics.c or whether it needs some
configure magick.

I also agree with FX that the function should be named something like
get_terminal_width().

In fact, I would argue that the Fortran version should be a wrapper
around this one to make the output consistent between the new Fortran
diagnostics and the old ones (*_1 variants) while the transition is in
progress, even if that means changing the current ordering for
Fortran. So far, there does not seem to be any reason to prefer one
ordering over the other, but whatever it is chosen, it would be better
to be consistent. Thus something like:

Comments

Dodji Seketeli Dec. 10, 2014, 1:12 p.m. UTC | #1
Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

[...]

> On 10 December 2014 at 12:10, Dodji Seketeli <dodji@redhat.com> wrote:

[...]

>> Manuel, was there a particular reason to avoid mentioning the COLUMNS
>> environment variable in the documentation?
>
> Not that I remember. Perhaps the documentation should say something
> like: "The line is truncated to fit into n characters only if the
> option -fmessage-length=n is given, or if the output is a TTY and the
> COLUMNS environment variable is set."

Agreed.  Thank you.

>>> (b) Should ioctl be always used or only for Fortran?
>>
>> I'd go for using it in the common diagnostics framework, unless there is
>> a sound motivated reason.  Manuel, do you remember why we didn't query the
>> TIOCGWINSZ ioctl property to get the terminal size when that capability
>> was available?
>
> I was not aware this possibility even existed.

Ok :-) Let's go for this then.

[...]

> Note that Fortran has this:
>
> #ifdef GWINSZ_IN_SYS_IOCTL
> # include <sys/ioctl.h>
> #endif
>
> Not sure if this is needed for diagnostics.c or whether it needs some
> configure magick.

I would guess that it should work to use that same #ifdef
GWINSZ_IN_SYS_IOCTL ... #endif snippet in diagnostics.c because, looking
at gcc/configure.ac, I see that we use the autoconf macro
AC_HEADER_TIOCGWINSZ and the autoconf documentation for that macro
reads:

 -- Macro: AC_HEADER_TIOCGWINSZ
     If the use of `TIOCGWINSZ' requires `<sys/ioctl.h>', then define
     `GWINSZ_IN_SYS_IOCTL'.  Otherwise `TIOCGWINSZ' can be found in
     `<termios.h>'.

     Use:

          #ifdef HAVE_TERMIOS_H
          # include <termios.h>
          #endif

          #ifdef GWINSZ_IN_SYS_IOCTL
          # include <sys/ioctl.h>
          #endif

I am not sure why we were not using the termios.h case though.

> I also agree with FX that the function should be named something like
> get_terminal_width().

Agreed as well.

> In fact, I would argue that the Fortran version should be a wrapper
> around this one to make the output consistent between the new Fortran
> diagnostics and the old ones (*_1 variants) while the transition is in
> progress, even if that means changing the current ordering for
> Fortran. So far, there does not seem to be any reason to prefer one
> ordering over the other, but whatever it is chosen, it would be better
> to be consistent.

I prefer that the environment variable taking precedent is better from a
usability standpoint because it's easier for the user to force the
behaviour she wants by setting an environment variable than by messing
up with some system-wide configuration what would change the output of
querying the TIOCGWINSZ property using an ioctl.

So the patch you (Manual) are proposing looks fine to me, with the
environment variable taking precedence, *if* that is fine for Fortran,
of course.

[...]

Cheers,
FX Dec. 10, 2014, 1:48 p.m. UTC | #2
> So the patch you (Manual) are proposing looks fine to me, with the
> environment variable taking precedence, *if* that is fine for Fortran,
> of course.

That seems fine to me, from the Fortran standpoint. COLUMNS is a bit of a special environment variable, which the shell (when it provides it) keeps synchronized to the terminal width anyway (as is the case for bash and zsh).

FX

Patch
diff mbox

Index: error.c
===================================================================
--- error.c    (revision 218410)
+++ error.c    (working copy)
@@ -78,7 +78,7 @@ 
 /* Determine terminal width (for trimming source lines in output).  */

 static int
-get_terminal_width (void)
+gfc_get_terminal_width (void)
 {
   /* Only limit the width if we're outputting to a terminal.  */
 #ifdef HAVE_UNISTD_H
@@ -85,26 +85,7 @@ 
   if (!isatty (STDERR_FILENO))
     return INT_MAX;
 #endif
-
-  /* Method #1: Use ioctl (not available on all systems).  */
-#ifdef TIOCGWINSZ
-  struct winsize w;
-  w.ws_col = 0;
-  if (ioctl (0, TIOCGWINSZ, &w) == 0 && w.ws_col > 0)
-    return w.ws_col;
-#endif
-
-  /* Method #2: Query environment variable $COLUMNS.  */
-  const char *p = getenv ("COLUMNS");
-  if (p)
-    {
-      int value = atoi (p);
-      if (value > 0)
-    return value;
-    }
-
-  /* If both fail, use reasonable default.  */
-  return 80;
+  return get_terminal_width ();
 }


@@ -113,7 +94,7 @@ 
 void
 gfc_error_init_1 (void)
 {
-  terminal_width = get_terminal_width ();
+  terminal_width = gfc_get_terminal_width ();
   errors = 0;
   warnings = 0;
   buffer_flag = 0;


The other "conflict" is that Fortran's default terminal_width if
everything fails is 80, whereas the common diagnostics defaults to
INT_MAX. If Fortran devs do not mind this new default (which is
already in place for all diagnostics using the new functions), then
the wrapper will make the output consistent. If they really want to
default to 80, then the Fortran code can still use the wrapper but do
"If (terminal_width == INT_MAX) terminal_width = 80" and call
diagnostic_set_caret_max_width(, terminal_width) to set the same value
to the new and old functions.

Finally, there is another use of getenv ("COLUMNS") in opts.c that
could use this new function:

Index: opts.c
===================================================================
--- opts.c    (revision 218410)
+++ opts.c    (working copy)
@@ -1231,18 +1231,8 @@ 
      the desired maximum width of the output.  */
   if (opts->x_help_columns == 0)
     {
-      const char *p;
-
-      p = getenv ("COLUMNS");
-      if (p != NULL)
-    {
-      int value = atoi (p);
-
-      if (value > 0)
-        opts->x_help_columns = value;
-    }
-
-      if (opts->x_help_columns == 0)
+      opts->x_help_columns = get_terminal_width ();
+      if (opts->x_help_columns == INT_MAX)
     /* Use a reasonable default.  */
     opts->x_help_columns = 80;
     }