public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
@ 2014-12-06 22:15 Tobias Burnus
  2014-12-07  9:38 ` FX
  2014-12-10 11:11 ` Dodji Seketeli
  0 siblings, 2 replies; 8+ messages in thread
From: Tobias Burnus @ 2014-12-06 22:15 UTC (permalink / raw)
  To: gcc-patches, gfortran, Manuel López-Ibáñez,
	Dodji Seketeli

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

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

[-- Attachment #2: diag.diff --]
[-- Type: text/x-patch, Size: 1272 bytes --]

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 28ef81c..9a29300 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -82,18 +82,27 @@ file_name_as_prefix (diagnostic_context *context, const char *f)
 
 
 \f
 /* Return the value of the getenv("COLUMNS") as an integer. If the
-   value is not set to a positive integer, then return INT_MAX.  */
+   value is not set to a positive integer, use ioctl to get the
+   terminal width. If it fails, return INT_MAX.  */
 static int
-getenv_columns (void)
+getenv_columns_and_termwidth (void)
 {
   const char * s = getenv ("COLUMNS");
   if (s != NULL) {
     int n = atoi (s);
     if (n > 0)
       return n;
   }
+
+#ifdef TIOCGWINSZ
+  struct winsize w;
+  w.ws_col = 0;
+  if (ioctl (0, TIOCGWINSZ, &w) == 0 && w.ws_col > 0)
+    return w.ws_col;
+#endif
+
   return INT_MAX;
 }
 
 /* Set caret_max_width to value.  */
@@ -102,9 +111,9 @@ 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);
+       ? getenv_columns_and_termwidth () - 1: INT_MAX);
   
   if (value <= 0) 
     value = INT_MAX;
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-06 22:15 [RFC] diagnostics.c: For terminals, restrict messages to terminal width? Tobias Burnus
@ 2014-12-07  9:38 ` FX
  2014-12-10 11:11 ` Dodji Seketeli
  1 sibling, 0 replies; 8+ messages in thread
From: FX @ 2014-12-07  9:38 UTC (permalink / raw)
  To: Tobias Burnus
  Cc: gcc-patches, gfortran, Manuel López-Ibáñez,
	Dodji Seketeli

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-06 22:15 [RFC] diagnostics.c: For terminals, restrict messages to terminal width? Tobias Burnus
  2014-12-07  9:38 ` FX
@ 2014-12-10 11:11 ` Dodji Seketeli
  2014-12-10 12:20   ` Manuel López-Ibáñez
  1 sibling, 1 reply; 8+ messages in thread
From: Dodji Seketeli @ 2014-12-10 11:11 UTC (permalink / raw)
  To: Tobias Burnus
  Cc: gcc-patches, gfortran, Manuel López-Ibáñez, FX

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!

-- 
		Dodji

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-10 11:11 ` Dodji Seketeli
@ 2014-12-10 12:20   ` Manuel López-Ibáñez
  2014-12-10 13:29     ` Dodji Seketeli
  0 siblings, 1 reply; 8+ messages in thread
From: Manuel López-Ibáñez @ 2014-12-10 12:20 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Tobias Burnus, gcc-patches, gfortran, FX

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:

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;
     }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-10 12:20   ` Manuel López-Ibáñez
@ 2014-12-10 13:29     ` Dodji Seketeli
  2014-12-10 13:48       ` FX
  0 siblings, 1 reply; 8+ messages in thread
From: Dodji Seketeli @ 2014-12-10 13:29 UTC (permalink / raw)
  To: Manuel López-Ibáñez
  Cc: Tobias Burnus, gcc-patches, gfortran, FX

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,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-10 13:29     ` Dodji Seketeli
@ 2014-12-10 13:48       ` FX
  2014-12-10 21:43         ` Tobias Burnus
  0 siblings, 1 reply; 8+ messages in thread
From: FX @ 2014-12-10 13:48 UTC (permalink / raw)
  To: Dodji Seketeli
  Cc: Manuel López-Ibáñez, Tobias Burnus, gcc-patches, gfortran

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-10 13:48       ` FX
@ 2014-12-10 21:43         ` Tobias Burnus
  2014-12-11  8:12           ` Dodji Seketeli
  0 siblings, 1 reply; 8+ messages in thread
From: Tobias Burnus @ 2014-12-10 21:43 UTC (permalink / raw)
  To: FX, Dodji Seketeli
  Cc: Manuel López-Ibáñez, gcc-patches, gfortran

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

Hi all,

I have now updated the patch, based on all comments and Manuel's patch. 
And bootstrapped it on x86-64-gnu-linux. If there are no more comments, 
I intent to commit it tomorrow. If it gets approved earlier, I will 
commit it earlier ;-)

Some comments from me are below.

FX wrote:
>> 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).

Ditto for me, although my feeling is that COLUMNS is only rarely used as 
it is does not seem to get automatically exported by the shells.


Manuel López-Ibáñez wrote:
> 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.

I think that's fine. Most Fortran source code is < 132 characters as 
maximally 132 characters are permitted by the standard (for free-form 
source code);* the number of cases, where it is longer, should be quite 
small and on average is should be even shorter. And terminal/editor 
widths of > 80 are also common, such that viewing a log there, also 
should work without wrapping.

(* gfortran doesn't count comments exceeding that limit.)

Tobias

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: termwidth.diff --]
[-- Type: text/x-patch; name="termwidth.diff", Size: 5563 bytes --]

2014-12-06  Tobias Burnus  <burnus@net-b.de>
	    Manuel López-Ibáñez  <manu@gcc.gnu.org>

gcc/
	* diagnostic.c (get_terminal_width): Renamed from getenv_columns,
	removed static, and additionally use ioctl to get width.
	(diagnostic_set_caret_max_width): Update call.
	* diagnostic.h (get_terminal_width): Add prototype.
	* opts.c (print_specific_help): Use it for x_help_columns.
	* doc/invoke.texi (fdiagnostics-show-caret): Document how the
	width is set.

gcc/fortran/
	* error.c (gfc_get_terminal_width): Renamed from
	get_terminal_width and use same-named common function.
	(gfc_error_init_1): Update call.

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 541e2fb..41101a2 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -33,6 +33,14 @@ along with GCC; see the file COPYING3.  If not see
 #include "diagnostic.h"
 #include "diagnostic-color.h"
 
+#ifdef HAVE_TERMIOS_H
+# include <termios.h>
+#endif
+
+#ifdef GWINSZ_IN_SYS_IOCTL
+# include <sys/ioctl.h>
+#endif
+
 #include <new>                     // For placement new.
 
 #define pedantic_warning_kind(DC)			\
@@ -81,9 +89,10 @@ file_name_as_prefix (diagnostic_context *context, const char *f)
 
 \f
 /* Return the value of the getenv("COLUMNS") as an integer. If the
-   value is not set to a positive integer, then return INT_MAX.  */
-static int
-getenv_columns (void)
+   value is not set to a positive integer, use ioctl to get the
+   terminal width. If it fails, return INT_MAX.  */
+int
+get_terminal_width (void)
 {
   const char * s = getenv ("COLUMNS");
   if (s != NULL) {
@@ -91,6 +100,14 @@ getenv_columns (void)
     if (n > 0)
       return n;
   }
+
+#ifdef TIOCGWINSZ
+  struct winsize w;
+  w.ws_col = 0;
+  if (ioctl (0, TIOCGWINSZ, &w) == 0 && w.ws_col > 0)
+    return w.ws_col;
+#endif
+
   return INT_MAX;
 }
 
@@ -101,7 +118,7 @@ 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);
+       ? get_terminal_width () - 1: INT_MAX);
   
   if (value <= 0) 
     value = INT_MAX;
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 807ce91..e699db8 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -298,6 +298,8 @@ void diagnostic_action_after_output (diagnostic_context *, diagnostic_t);
 
 void diagnostic_file_cache_fini (void);
 
+int get_terminal_width (void);
+
 /* Expand the location of this diagnostic. Use this function for consistency. */
 
 static inline expanded_location
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d2f3c79..40eb8b6 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3187,7 +3187,10 @@ option is known to the diagnostic machinery).  Specifying the
 @opindex fdiagnostics-show-caret
 By default, each diagnostic emitted includes the original source line
 and a caret '^' indicating the column.  This option suppresses this
-information.
+information.  The source line is truncated to @var{n} characters, if
+the @option{-fmessage-length=n} is given.  When the output is done
+to the terminal, the width is limited to the width given by the
+@env{COLUMNS} environment variable or, if not set, to the terminal width.
 
 @end table
 
diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
index 541a799..75407dc 100644
--- a/gcc/fortran/error.c
+++ b/gcc/fortran/error.c
@@ -30,14 +30,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "flags.h"
 #include "gfortran.h"
 
-#ifdef HAVE_TERMIOS_H
-# include <termios.h>
-#endif
-
-#ifdef GWINSZ_IN_SYS_IOCTL
-# include <sys/ioctl.h>
-#endif
-
 #include "diagnostic.h"
 #include "diagnostic-color.h"
 #include "tree-diagnostic.h" /* tree_diagnostics_defaults */
@@ -91,33 +83,9 @@ gfc_pop_suppress_errors (void)
 /* 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
-  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 isatty (STDERR_FILENO) ? get_terminal_width () : INT_MAX;
 }
 
 
@@ -126,7 +94,7 @@ get_terminal_width (void)
 void
 gfc_error_init_1 (void)
 {
-  terminal_width = get_terminal_width ();
+  terminal_width = gfc_get_terminal_width ();
   errors = 0;
   warnings = 0;
   gfc_buffer_error (false);
diff --git a/gcc/opts.c b/gcc/opts.c
index 1b4f97e..34a42a5 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1231,18 +1231,8 @@ print_specific_help (unsigned int include_flags,
      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;
     }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] diagnostics.c: For terminals, restrict messages to terminal width?
  2014-12-10 21:43         ` Tobias Burnus
@ 2014-12-11  8:12           ` Dodji Seketeli
  0 siblings, 0 replies; 8+ messages in thread
From: Dodji Seketeli @ 2014-12-11  8:12 UTC (permalink / raw)
  To: Tobias Burnus
  Cc: FX, Manuel López-Ibáñez, gcc-patches, gfortran

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

> 2014-12-06  Tobias Burnus  <burnus@net-b.de>
> 	    Manuel L³pez-Ib¡±ez  <manu@gcc.gnu.org>
>
> gcc/
> 	* diagnostic.c (get_terminal_width): Renamed from getenv_columns,
> 	removed static, and additionally use ioctl to get width.
> 	(diagnostic_set_caret_max_width): Update call.
> 	* diagnostic.h (get_terminal_width): Add prototype.
> 	* opts.c (print_specific_help): Use it for x_help_columns.
> 	* doc/invoke.texi (fdiagnostics-show-caret): Document how the
> 	width is set.
>
> gcc/fortran/
> 	* error.c (gfc_get_terminal_width): Renamed from
> 	get_terminal_width and use same-named common function.
> 	(gfc_error_init_1): Update call.

The diagnostics infrastructure changes are OK for me.  Thanks!

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-12-11  8:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-06 22:15 [RFC] diagnostics.c: For terminals, restrict messages to terminal width? Tobias Burnus
2014-12-07  9:38 ` FX
2014-12-10 11:11 ` Dodji Seketeli
2014-12-10 12:20   ` Manuel López-Ibáñez
2014-12-10 13:29     ` Dodji Seketeli
2014-12-10 13:48       ` FX
2014-12-10 21:43         ` Tobias Burnus
2014-12-11  8:12           ` Dodji Seketeli

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