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

This patch fixes a Fortran diagnostic "regression".

With the current common diagnostic, the width shown with caret 
diagnostic is determined by:

     case OPT_fmessage_length_:
       pp_set_line_maximum_length (dc->printer, value);
       diagnostic_set_caret_max_width (dc, value);

plus

  diagnostic_set_caret_max_width (diagnostic_context *context, int value)
  {
    /* One minus to account for the leading empty space.  */
    value = value ? value - 1
      : (isatty (fileno (pp_buffer (context->printer)->stream))
        ? getenv_columns () - 1: INT_MAX);

    if (value <= 0)
      value = INT_MAX;

    context->caret_max_width = value;
  }

where getenv_columns looks at the environment variable COLUMNS.

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.)

On most terminals, which I tried, COLUMNS does not seem to be set. In 
Fortran, error.c's get_terminal_width has a similar check, but 
additionally it uses ioctl to determine the terminal width.

I think with caret diagnostics, it is useful not to exceed the terminal 
width as having several "empty" lines before the "^" does not really 
improve the readability. Thus, I would propose to additionally use 
ioctl. Which rises two questions: (a) Should the COLUMNS environment 
variable or ioctl have a higher priority? [Fortran ranks ioctl higher; 
in the patch, for backward compatibilty, I rank COLUMNS higher.] (b) 
Should ioctl be always used or only for Fortran?

Comments?

Tobias

OK. But if we rename the function, why not simply terminal_width() ?

FX



> Le 6 déc. 2014 à 23:14, Tobias Burnus <burnus@net-b.de> a écrit :
> 
> This patch fixes a Fortran diagnostic "regression".
> 
> With the current common diagnostic, the width shown with caret diagnostic is determined by:
> 
>    case OPT_fmessage_length_:
>      pp_set_line_maximum_length (dc->printer, value);
>      diagnostic_set_caret_max_width (dc, value);
> 
> plus
> 
> diagnostic_set_caret_max_width (diagnostic_context *context, int value)
> {
>   /* One minus to account for the leading empty space.  */
>   value = value ? value - 1
>     : (isatty (fileno (pp_buffer (context->printer)->stream))
>       ? getenv_columns () - 1: INT_MAX);
> 
>   if (value <= 0)
>     value = INT_MAX;
> 
>   context->caret_max_width = value;
> }
> 
> where getenv_columns looks at the environment variable COLUMNS.
> 
> 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.)
> 
> On most terminals, which I tried, COLUMNS does not seem to be set. In Fortran, error.c's get_terminal_width has a similar check, but additionally it uses ioctl to determine the terminal width.
> 
> I think with caret diagnostics, it is useful not to exceed the terminal width as having several "empty" lines before the "^" does not really improve the readability. Thus, I would propose to additionally use ioctl. Which rises two questions: (a) Should the COLUMNS environment variable or ioctl have a higher priority? [Fortran ranks ioctl higher; in the patch, for backward compatibilty, I rank COLUMNS higher.] (b) Should ioctl be always used or only for Fortran?
> 
> Comments?
> 
> Tobias
> <diag.diff>

Hello Tobias,

Thank you for this patch.  I have a few comments about it below.  Just
as a heads-up, I am asking questions to Manuel in there, as well as
referring to comments from FX's.  Please read below.

Tobias Burnus <burnus@net-b.de> writes:

> This patch fixes a Fortran diagnostic "regression".
>
> With the current common diagnostic, the width shown with caret
> diagnostic is determined by:
>
>     case OPT_fmessage_length_:
>       pp_set_line_maximum_length (dc->printer, value);
>       diagnostic_set_caret_max_width (dc, value);
>
> plus
>
>  diagnostic_set_caret_max_width (diagnostic_context *context, int value)
>  {
>    /* One minus to account for the leading empty space.  */
>    value = value ? value - 1
>      : (isatty (fileno (pp_buffer (context->printer)->stream))
>        ? getenv_columns () - 1: INT_MAX);
>
>    if (value <= 0)
>      value = INT_MAX;
>
>    context->caret_max_width = value;
>  }
>
> where getenv_columns looks at the environment variable COLUMNS.
>
> 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?

> On most terminals, which I tried, COLUMNS does not seem to be set. In
> Fortran, error.c's get_terminal_width has a similar check, but
> additionally it uses ioctl to determine the terminal width.
>
> I think with caret diagnostics, it is useful not to exceed the
> terminal width as having several "empty" lines before the "^" does not
> really improve the readability. Thus, I would propose to additionally
> use ioctl. Which rises two questions: (a) Should the COLUMNS
> environment variable or ioctl have a higher priority? [Fortran ranks
> ioctl higher; in the patch, for backward compatibilty, I rank COLUMNS
> higher.]

I agree.

> (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?

> Comments?

If the change comes with ChangeLog, passes bootstrap and nobody else
objects, I pre-approve this patch.

Thanks!