Re: [PATCH] Caret diagnostics

2012-04-12 Thread Tom Tromey
 Gaby == Gabriel Dos Reis g...@integrable-solutions.net writes:

Manuel So, in fact, libcpp is buggy for not implementing this (as can be seen
Manuel in emacs). If/When libcpp is fixed, the column info will be correct
Manuel for tabs. And then, one may care about printing tabs as anything
Manuel different from a single space. But until then, a single space is a
Manuel good as anything. In fact this is what is done in c-ppoutput.c.

Gaby I would not necessarily say that libcpp is buggy in this aspect.
Gaby However, I do agree that removing the tab expansion simplifies
Gaby the code and is better.

Unfortunately I think it really is buggy.  Other GNU programs, including
Emacs, already conform to the GCS here.  Arguably the GCS made the wrong
decision, but it is already baked in all over.

Tom


Re: [PATCH] Caret diagnostics

2012-04-12 Thread Gabriel Dos Reis
On Thu, Apr 12, 2012 at 11:53 AM, Tom Tromey tro...@redhat.com wrote:
 Gaby == Gabriel Dos Reis g...@integrable-solutions.net writes:

 Manuel So, in fact, libcpp is buggy for not implementing this (as can be seen
 Manuel in emacs). If/When libcpp is fixed, the column info will be correct
 Manuel for tabs. And then, one may care about printing tabs as anything
 Manuel different from a single space. But until then, a single space is a
 Manuel good as anything. In fact this is what is done in c-ppoutput.c.

 Gaby I would not necessarily say that libcpp is buggy in this aspect.
 Gaby However, I do agree that removing the tab expansion simplifies
 Gaby the code and is better.

 Unfortunately I think it really is buggy.  Other GNU programs, including
 Emacs, already conform to the GCS here.  Arguably the GCS made the wrong
 decision, but it is already baked in all over.

OK.

-- Gaby


Re: [PATCH] Caret diagnostics

2012-04-11 Thread H.J. Lu
On Tue, Apr 10, 2012 at 1:33 PM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 10 April 2012 21:41, Manuel López-Ibáñez lopeziba...@gmail.com wrote:
 On 10 April 2012 21:31, Jason Merrill ja...@redhat.com wrote:
 On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:

 +  max_width = context-caret_max_width;
 +  if (max_width= 0)
 +    max_width = INT_MAX;


 I don't think we need the test here; diagnostic_set_caret_max_width should
 make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.

 It was just a bit of defensive programming, since nothing stops anyone
 to set the value directly. But I will remove it.

 On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.


 Hmm.  I don't know if there's any reliable way to query tab stops; the it
 termcap/terminfo capability tells you what it was originally (presumably 8),
 but it might have changed.

 No idea either, but it is in fact easier to print tabs a single
 spaces. This simplifies the code a lot, as pointed by Gabriel. So if
 you also prefer the simpler version, I am fine with committing that
 one (it also saves space in the output).

 A new version of the patch. It removes the special handling of tabs.
 And abstracts out the adjusting of the line as Gabriel suggested. I am
 not sure what else could be abstracted out. The function is much
 shorter than other functions in diagnostic.c.

 This patch builds and I am running a full boostrap+check now.

 OK if it passes?

 Cheers,

 Manuel.


 2012-04-05  Manuel López-Ibáñez  m...@gcc.gnu.org

        PR 24985
 libstdc++-v3/
        * testsuite/lib/prune.exp: Handle caret.
 libmudflap/
        * testsuite/lib/libmudflap.exp: Handle caret.
 gcc/
        * diagnostic.h (show_caret): Declare.
        (caret_max_width): Declare.
        (diagnostic_show_locus): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
        (getenv_columns): New.
        (adjust_line): New.
        (diagnostic_set_caret_max_width): New.
        * input.c (read_line): New.
        (location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
        * pretty-print.h (pp_get_prefix): New.
        (pp_base_get_prefix): New.
        * common.opt (fdiagnostics-show-caret): New option.
        * doc/invoke.texi (fdiagnostics-show-caret): Document it.
 testsuite/
        * lib/prune.exp: Add -fno-diagnostics-show-caret.

It breaks library tests:

ERROR: tcl error sourcing
../../../../src-trunk/boehm-gc/testsuite/../../gcc/testsuite/lib/prune.exp.
ERROR: tcl error sourcing
../../../../src-trunk/libgomp/testsuite/../../gcc/testsuite/lib/prune.exp.
ERROR: tcl error sourcing
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.

RROR: tcl error sourcing
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.
can't read TEST_ALWAYS_FLAGS: no such variable
while executing
set TEST_ALWAYS_FLAGS -fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS
(file 
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp
line 20)
invoked from within
source 
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp
(uplevel body line 1)
invoked from within
uplevel #0 source
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp
invoked from within
catch uplevel #0 source $file
make[8]: *** [check-DEJAGNU] Error 1

-- 
H.J.


Re: [PATCH] Caret diagnostics

2012-04-11 Thread Manuel López-Ibáñez
In 11 April 2012 18:40, H.J. Lu hjl.to...@gmail.com wrote:

 It breaks library tests:

 ERROR: tcl error sourcing
 ../../../../src-trunk/boehm-gc/testsuite/../../gcc/testsuite/lib/prune.exp.
 ERROR: tcl error sourcing
 ../../../../src-trunk/libgomp/testsuite/../../gcc/testsuite/lib/prune.exp.
 ERROR: tcl error sourcing
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.

 RROR: tcl error sourcing
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.
 can't read TEST_ALWAYS_FLAGS: no such variable
    while executing
 set TEST_ALWAYS_FLAGS -fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS
    (file 
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp
 line 20)
    invoked from within
 source 
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp
    (uplevel body line 1)
    invoked from within
 uplevel #0 source
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp
    invoked from within
 catch uplevel #0 source $file
 make[8]: *** [check-DEJAGNU] Error 1

contrib/compare_tests does not seem to detect this :-(

So how can I say in tcl if not defined VAR then set VAR ?

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-11 Thread Andrew Pinski
On Wed, Apr 11, 2012 at 11:15 AM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 So how can I say in tcl if not defined VAR then set VAR ?

if ![info exists VAR] {
  set VAR XXX
}

Thanks,
Andrew Pinski


Re: [PATCH] Caret diagnostics

2012-04-11 Thread Mike Stump
On Apr 11, 2012, at 11:15 AM, Manuel López-Ibáñez wrote:
 contrib/compare_tests does not seem to detect this :-(

I'd bet, either, there were no tests after that point, or, compare_tests 
pointed it out.  Feel free to send the two .sum files and and I will 
investigate if you think that isn't the case.


Re: [PATCH] Caret diagnostics

2012-04-11 Thread Manuel López-Ibáñez
On 11 April 2012 18:40, H.J. Lu hjl.to...@gmail.com wrote:

 It breaks library tests:

 ERROR: tcl error sourcing
 ../../../../src-trunk/boehm-gc/testsuite/../../gcc/testsuite/lib/prune.exp.
 ERROR: tcl error sourcing
 ../../../../src-trunk/libgomp/testsuite/../../gcc/testsuite/lib/prune.exp.
 ERROR: tcl error sourcing


Hopefully fixed by this patch:

Index: libgomp/ChangeLog
===
--- libgomp/ChangeLog   (revision 186352)
+++ libgomp/ChangeLog   (revision 186353)
@@ -1,3 +1,7 @@
+2012-04-11  Manuel López-Ibáñez  m...@gcc.gnu.org
+
+   * testsuite/lib/libgomp.exp: Add -fno-diagnostics-show-caret.
+
 2012-03-31  H.J. Lu  hongjiu...@intel.com

PR bootstrap/52812
Index: libgomp/testsuite/lib/libgomp.exp
===
--- libgomp/testsuite/lib/libgomp.exp   (revision 186352)
+++ libgomp/testsuite/lib/libgomp.exp   (revision 186353)
@@ -161,6 +161,9 @@
 # error-message parsing machinery.
 lappend ALWAYS_CFLAGS additional_flags=-fmessage-length=0

+# Disable caret
+lappend ALWAYS_CFLAGS additional_flags=-fno-diagnostics-show-caret
+
 # And, gee, turn on OpenMP.
 lappend ALWAYS_CFLAGS additional_flags=-fopenmp
 }
Index: gcc/testsuite/lib/prune.exp
===
--- gcc/testsuite/lib/prune.exp (revision 186352)
+++ gcc/testsuite/lib/prune.exp (revision 186353)
@@ -17,6 +17,9 @@

 # Prune messages from gcc that aren't useful.

+if ![info exists TEST_ALWAYS_FLAGS] {
+set TEST_ALWAYS_FLAGS 
+}
 set TEST_ALWAYS_FLAGS -fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS

 proc prune_gcc_output { text } {
Index: gcc/testsuite/ChangeLog
===
--- gcc/testsuite/ChangeLog (revision 186352)
+++ gcc/testsuite/ChangeLog (revision 186353)
@@ -1,3 +1,7 @@
+2012-04-11  Manuel López-Ibáñez  m...@gcc.gnu.org
+
+   * lib/prune.exp (TEST_ALWAYS_FLAGS): If undefined, set to empty.
+
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.


Committed after testing as revision 186353.

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Manuel López-Ibáñez
On 10 April 2012 00:28, Jason Merrill ja...@redhat.com wrote:
 On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:

 * It uses  the default cutoff as max_width, whatever it is (as
 controlled by -fmessage-length).
 * It uses the pretty-printer. The text cannot (should not) wrap
 because we still print only max_width chars at most.


 Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
 to use COLUMNS to limit how much of the source we print.

Like this?

2012-04-05  Manuel López-Ibáñez  m...@gcc.gnu.org

PR 24985
libstdc++-v3/
* testsuite/lib/prune.exp: Handle caret.
libmudflap/
* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
* diagnostic.h (show_caret): Declare.
(caret_max_width): Declare.
(diagnostic_show_locus): Declare.
(diagnostic_set_caret_max_width): Declare.
* diagnostic.c (diagnostic_initialize): Initialize to false.
(diagnostic_show_locus): New.
(diagnostic_report_diagnostic): Call it.
(getenv_columns): New.
(diagnostic_set_caret_max_width): New.
* input.c (read_line): New.
(location_get_source_line): New.
* input.h (location_get_source_line): Declare.
* toplev.c (general_init): Initialize show_caret from options.
* dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
* opts.c (common_handle_option): Likewise.
* pretty-print.h (pp_get_prefix): New.
(pp_base_get_prefix): New.
* common.opt (fdiagnostics-show-caret): New option.
* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
* lib/prune.exp: Add -fno-diagnostics-show-caret.


caret-diagnostics-20120410.diff
Description: Binary data


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Gabriel Dos Reis
On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 10 April 2012 00:28, Jason Merrill ja...@redhat.com wrote:
 On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:

 * It uses  the default cutoff as max_width, whatever it is (as
 controlled by -fmessage-length).
 * It uses the pretty-printer. The text cannot (should not) wrap
 because we still print only max_width chars at most.


 Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
 to use COLUMNS to limit how much of the source we print.

 Like this?

There is a novelty in this new version that I don't think we discussed
before: automatic expansion of tabs to 8 hard space characters.  That
number should not be hardcoded as there is no reason to believe a tab
character always expands to 8 space characters.  You should check
the environment first; if not present the default expansion number should
be a symbolic constant as opposed to a magic number sprinkled all over the
places.  I would also encourage you to introduce more abstraction to
reduce the size of diagnostic_show_locus.


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Jason Merrill

On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:

+  max_width = context-caret_max_width;
+  if (max_width= 0)
+max_width = INT_MAX;


I don't think we need the test here; diagnostic_set_caret_max_width 
should make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.


On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:

There is a novelty in this new version that I don't think we discussed
before: automatic expansion of tabs to 8 hard space characters.  That
number should not be hardcoded as there is no reason to believe a tab
character always expands to 8 space characters.  You should check
the environment first; if not present the default expansion number should
be a symbolic constant as opposed to a magic number sprinkled all over the
places.


Hmm.  I don't know if there's any reliable way to query tab stops; the 
it termcap/terminfo capability tells you what it was originally 
(presumably 8), but it might have changed.


Jason


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Manuel López-Ibáñez
On 10 April 2012 20:52, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 10 April 2012 00:28, Jason Merrill ja...@redhat.com wrote:
 On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:

 * It uses  the default cutoff as max_width, whatever it is (as
 controlled by -fmessage-length).
 * It uses the pretty-printer. The text cannot (should not) wrap
 because we still print only max_width chars at most.


 Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
 to use COLUMNS to limit how much of the source we print.

 Like this?

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.  I would also encourage you to introduce more abstraction to
 reduce the size of diagnostic_show_locus.

There is no novelty, it has been there from the beginning, and now I
realize it was a mistake to do the extra work to implement this.

The GCS says: Calculate column numbers assuming that space and all
ASCII printing characters have equal width, and assuming tab stops
every 8 columns
http://www.gnu.org/prep/standards/standards.html#Errors

So, in fact, libcpp is buggy for not implementing this (as can be seen
in emacs). If/When libcpp is fixed, the column info will be correct
for tabs. And then, one may care about printing tabs as anything
different from a single space. But until then, a single space is a
good as anything. In fact this is what is done in c-ppoutput.c.

As can be seen below, this simplifies a lot the function, so I
appreciate the suggestion. If you have suggestions for removing other
code, I will be happy to implement them. The less code, the less
potential for bugs.

Cheers,

Manuel.

+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+void
+diagnostic_show_locus (diagnostic_context * context,
+  const diagnostic_info *diagnostic)
+{
+  const char *line;
+  char *buffer;
+  expanded_location s;
+  int real_width;
+  int max_width;
+  const char *saved_prefix;
+  int right_margin = 10;
+
+  if (!context-show_caret
+  || diagnostic-location = BUILTINS_LOCATION)
+return;
+
+  s = expand_location(diagnostic-location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+return;
+
+  real_width = strlen (line);
+
+  max_width = context-caret_max_width;
+  if (max_width = 0)
+max_width = INT_MAX;
+
+  /* If the line is longer than max_width and the column is too close
+ to max_width, skip part of the line until it is not so close.  */
+  right_margin = MIN(real_width - s.column, right_margin);
+  right_margin = max_width - right_margin;
+  if (real_width = max_width  s.column  right_margin)
+{
+  line += s.column - right_margin;
+  s.column = right_margin;
+}
+
+  pp_newline (context-printer);
+  saved_prefix = pp_get_prefix (context-printer);
+  pp_set_prefix (context-printer, NULL);
+  pp_character (context-printer, ' ');
+  while (max_width  0  *line != '\0')
+{
+  char c = *line == '\t' ? ' ' : *line;
+  pp_character (context-printer, c);
+  max_width--;
+  line++;
+}
+  pp_newline (context-printer);
+  /* pp_printf does not implement %*c.  */
+  buffer = XALLOCAVEC (char, s.column + 3);
+  snprintf (buffer, s.column + 3,  %*c, s.column, '^');
+  pp_string (context-printer, buffer);
+  pp_set_prefix (context-printer, saved_prefix);
+}
+


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Manuel López-Ibáñez
On 10 April 2012 21:31, Jason Merrill ja...@redhat.com wrote:
 On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:

 +  max_width = context-caret_max_width;
 +  if (max_width= 0)
 +    max_width = INT_MAX;


 I don't think we need the test here; diagnostic_set_caret_max_width should
 make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.

It was just a bit of defensive programming, since nothing stops anyone
to set the value directly. But I will remove it.

 On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.


 Hmm.  I don't know if there's any reliable way to query tab stops; the it
 termcap/terminfo capability tells you what it was originally (presumably 8),
 but it might have changed.

No idea either, but it is in fact easier to print tabs a single
spaces. This simplifies the code a lot, as pointed by Gabriel. So if
you also prefer the simpler version, I am fine with committing that
one (it also saves space in the output).

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Manuel López-Ibáñez
On 10 April 2012 21:41, Manuel López-Ibáñez lopeziba...@gmail.com wrote:
 On 10 April 2012 21:31, Jason Merrill ja...@redhat.com wrote:
 On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:

 +  max_width = context-caret_max_width;
 +  if (max_width= 0)
 +    max_width = INT_MAX;


 I don't think we need the test here; diagnostic_set_caret_max_width should
 make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.

 It was just a bit of defensive programming, since nothing stops anyone
 to set the value directly. But I will remove it.

 On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.


 Hmm.  I don't know if there's any reliable way to query tab stops; the it
 termcap/terminfo capability tells you what it was originally (presumably 8),
 but it might have changed.

 No idea either, but it is in fact easier to print tabs a single
 spaces. This simplifies the code a lot, as pointed by Gabriel. So if
 you also prefer the simpler version, I am fine with committing that
 one (it also saves space in the output).

A new version of the patch. It removes the special handling of tabs.
And abstracts out the adjusting of the line as Gabriel suggested. I am
not sure what else could be abstracted out. The function is much
shorter than other functions in diagnostic.c.

This patch builds and I am running a full boostrap+check now.

OK if it passes?

Cheers,

Manuel.


2012-04-05  Manuel López-Ibáñez  m...@gcc.gnu.org

PR 24985
libstdc++-v3/
* testsuite/lib/prune.exp: Handle caret.
libmudflap/
* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
* diagnostic.h (show_caret): Declare.
(caret_max_width): Declare.
(diagnostic_show_locus): Declare.
* diagnostic.c (diagnostic_initialize): Initialize to false.
(diagnostic_show_locus): New.
(diagnostic_report_diagnostic): Call it.
(getenv_columns): New.
(adjust_line): New.
(diagnostic_set_caret_max_width): New.
* input.c (read_line): New.
(location_get_source_line): New.
* input.h (location_get_source_line): Declare.
* toplev.c (general_init): Initialize show_caret from options.
* dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
* opts.c (common_handle_option): Likewise.
* pretty-print.h (pp_get_prefix): New.
(pp_base_get_prefix): New.
* common.opt (fdiagnostics-show-caret): New option.
* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
* lib/prune.exp: Add -fno-diagnostics-show-caret.


caret-diagnostics-20120411.diff
Description: Binary data


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Mike Stump
On Apr 10, 2012, at 11:52 AM, Gabriel Dos Reis wrote:
 On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 10 April 2012 00:28, Jason Merrill ja...@redhat.com wrote:
 On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
 
 * It uses  the default cutoff as max_width, whatever it is (as
 controlled by -fmessage-length).
 * It uses the pretty-printer. The text cannot (should not) wrap
 because we still print only max_width chars at most.
 
 
 Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
 to use COLUMNS to limit how much of the source we print.
 
 Like this?
 
 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.  I would also encourage you to introduce more abstraction to
 reduce the size of diagnostic_show_locus.

Knobs have a cost.  Having 1000s of command line options doesn't really make a 
product better.  Our job is to provide what we must and omit what we can.  You 
argue for a knob that no user has asked for or expressed a need for.  I think 
we should have a much higher bar than this.  The reality is, tabs are 8, 
period.  There was a time went editors managed tabs stops by changing column 
into which a tab would move, this ability taken from devices called typewriters 
which had a little metal bit that you moved around where you wanted tab stops.  
The days of there devices called typewriters are over, we no longer cater to 
them.  Today, editors can manage what the key called TAB does, and its 
semantics are disconnected from the file save format.  This file format can be 
set at 8, while the edit function is indent 2, 4, 8 or as complex as what emacs 
does with c-mode.  It is a bad idea to have a file format tab is other than 8, 
and we should encourage all but legacy people to convert to 8.  We do this by 
having a wiki entry and explains the use of pr, and how to set the file save 
format to 8, if we need to.  For legacy people, they don't _want_ a new 
compiler, pretty much by definition.  So, I'd argue for a symbol constant, but, 
a constant nonetheless.  If a large user _had_ serious needs for something 
other than 8, _they_ can change the compiler to:

  #define TAB_COLS (getenv(TABS) ? atoi (genenv(TABS)) : 8)

if they want.  I don't think we should have a knob for this.


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Gabriel Dos Reis
On Tue, Apr 10, 2012 at 5:23 PM, Mike Stump mikest...@comcast.net wrote:
 On Apr 10, 2012, at 11:52 AM, Gabriel Dos Reis wrote:
 On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 10 April 2012 00:28, Jason Merrill ja...@redhat.com wrote:
 On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:

 * It uses  the default cutoff as max_width, whatever it is (as
 controlled by -fmessage-length).
 * It uses the pretty-printer. The text cannot (should not) wrap
 because we still print only max_width chars at most.


 Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
 to use COLUMNS to limit how much of the source we print.

 Like this?

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.  I would also encourage you to introduce more abstraction to
 reduce the size of diagnostic_show_locus.

 Knobs have a cost.  Having 1000s of command line options doesn't really make 
 a product better.  Our job is to provide what we must and omit what we can.  
 You argue for a knob that no user has asked for or expressed a need for.  I 
 think we should have a much higher bar than this.  The reality is, tabs are 
 8, period.  There was a time went editors managed tabs stops by changing 
 column into which a tab would move, this ability taken from devices called 
 typewriters which had a little metal bit that you moved around where you 
 wanted tab stops.  The days of there devices called typewriters are over, we 
 no longer cater to them.  Today, editors can manage what the key called TAB 
 does, and its semantics are disconnected from the file save format.  This 
 file format can be set at 8, while the edit function is indent 2, 4, 8 or as 
 complex as what emacs does with c-mode.  It is a bad idea to have a file 
 format tab is other than 8, and we should encourage all but legacy people to 
 convert to 8.  We do this by having a wiki entry and explains the use of pr, 
 and how to set the file save format to 8, if we need to.  For legacy people, 
 they don't _want_ a new compiler, pretty much by definition.  So, I'd argue 
 for a symbol constant, but, a constant nonetheless.  If a large user _had_ 
 serious needs for something other than 8, _they_ can change the compiler to:

  #define TAB_COLS (getenv(TABS) ? atoi (genenv(TABS)) : 8)

 if they want.  I don't think we should have a knob for this.

This is fully mystifying to me and I still do not understand what it
is all about after reading it 8 times.

- Gaby


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Mike Stump
On Apr 10, 2012, at 4:37 PM, Gabriel Dos Reis wrote:
 You should check the environment first


 I don't think we should have a knob for this.

 This is fully mystifying to me and I still do not understand what it is all 
 about after reading it 8 times.

Let me rephrase, I disagree, we should not check the environment first.

I assume, you meant checking the environment, git getenv, for some named 
variable to control the expansion of tabs.  Yes?


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Jason Merrill

On 04/10/2012 04:33 PM, Manuel López-Ibáñez wrote:

A new version of the patch. It removes the special handling of tabs.
And abstracts out the adjusting of the line as Gabriel suggested. I am
not sure what else could be abstracted out. The function is much
shorter than other functions in diagnostic.c.

This patch builds and I am running a full boostrap+check now.

OK if it passes?


OK.

Jason



Re: [PATCH] Caret diagnostics

2012-04-10 Thread Gabriel Dos Reis
On Tue, Apr 10, 2012 at 7:54 PM, Mike Stump mikest...@comcast.net wrote:
 On Apr 10, 2012, at 4:37 PM, Gabriel Dos Reis wrote:
 You should check the environment first


 I don't think we should have a knob for this.

 This is fully mystifying to me and I still do not understand what it is all 
 about after reading it 8 times.

 Let me rephrase, I disagree, we should not check the environment first.

 I assume, you meant checking the environment, git getenv, for some named 
 variable to control the expansion of tabs.  Yes?

Yes, because tabs expansion is very much environment dependent.


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Gabriel Dos Reis
On Tue, Apr 10, 2012 at 2:37 PM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 10 April 2012 20:52, Gabriel Dos Reis g...@integrable-solutions.net 
 wrote:
 On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 10 April 2012 00:28, Jason Merrill ja...@redhat.com wrote:
 On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:

 * It uses  the default cutoff as max_width, whatever it is (as
 controlled by -fmessage-length).
 * It uses the pretty-printer. The text cannot (should not) wrap
 because we still print only max_width chars at most.


 Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
 to use COLUMNS to limit how much of the source we print.

 Like this?

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.  I would also encourage you to introduce more abstraction to
 reduce the size of diagnostic_show_locus.

 There is no novelty, it has been there from the beginning, and now I
 realize it was a mistake to do the extra work to implement this.

I wonder how I missed it in the previous reviews.


 The GCS says: Calculate column numbers assuming that space and all
 ASCII printing characters have equal width, and assuming tab stops
 every 8 columns
 http://www.gnu.org/prep/standards/standards.html#Errors

That is GNU coding convention for GNU codes.  We do not require that
programs GCC accept have to adhere to GNU coding convention, not
should we.  An important aspect of diagnostic with caret is that we are
writing back to the user what she wrote, not what GNU wanted her
to write.

 So, in fact, libcpp is buggy for not implementing this (as can be seen
 in emacs). If/When libcpp is fixed, the column info will be correct
 for tabs. And then, one may care about printing tabs as anything
 different from a single space. But until then, a single space is a
 good as anything. In fact this is what is done in c-ppoutput.c.


I would not necessarily say that libcpp is buggy in this aspect.
However, I do agree that removing the tab expansion simplifies
the code and is better.


Re: [PATCH] Caret diagnostics

2012-04-10 Thread Gabriel Dos Reis
On Tue, Apr 10, 2012 at 3:33 PM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 10 April 2012 21:41, Manuel López-Ibáñez lopeziba...@gmail.com wrote:
 On 10 April 2012 21:31, Jason Merrill ja...@redhat.com wrote:
 On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:

 +  max_width = context-caret_max_width;
 +  if (max_width= 0)
 +    max_width = INT_MAX;


 I don't think we need the test here; diagnostic_set_caret_max_width should
 make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.

 It was just a bit of defensive programming, since nothing stops anyone
 to set the value directly. But I will remove it.

 On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:

 There is a novelty in this new version that I don't think we discussed
 before: automatic expansion of tabs to 8 hard space characters.  That
 number should not be hardcoded as there is no reason to believe a tab
 character always expands to 8 space characters.  You should check
 the environment first; if not present the default expansion number should
 be a symbolic constant as opposed to a magic number sprinkled all over the
 places.


 Hmm.  I don't know if there's any reliable way to query tab stops; the it
 termcap/terminfo capability tells you what it was originally (presumably 8),
 but it might have changed.

 No idea either, but it is in fact easier to print tabs a single
 spaces. This simplifies the code a lot, as pointed by Gabriel. So if
 you also prefer the simpler version, I am fine with committing that
 one (it also saves space in the output).

 A new version of the patch. It removes the special handling of tabs.
 And abstracts out the adjusting of the line as Gabriel suggested. I am
 not sure what else could be abstracted out. The function is much
 shorter than other functions in diagnostic.c.

 This patch builds and I am running a full boostrap+check now.

 OK if it passes?

OK.


Re: [PATCH] Caret diagnostics

2012-04-09 Thread Manuel López-Ibáñez
Attached a new version of the patch:

* It uses  the default cutoff as max_width, whatever it is (as
controlled by -fmessage-length).
* It uses the pretty-printer. The text cannot (should not) wrap
because we still print only max_width chars at most.
* It removes unnecessary prune patterns.

Bootstrapped and regression tested with languages=all,ada,obj-c++.

Cheers,

Manuel.

2012-04-09  Manuel López-Ibáñez  m...@gcc.gnu.org
PR 24985
libstdc++-v3/
* testsuite/lib/prune.exp: Handle caret.
libmudflap/
* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
* diagnostic.h (show_caret): Declare.
(diagnostic_show_locus): Declare.
* diagnostic.c (diagnostic_initialize): Initialize to false.
(diagnostic_show_locus): New.
(diagnostic_report_diagnostic): Call it.
* input.c (read_line): New.
(location_get_source_line): New.
* input.h (location_get_source_line): Declare.
* toplev.c (general_init): Initialize show_caret from options.
* dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
* opts.c (common_handle_option): Likewise.
* pretty-print.h (pp_get_prefix): New.
(pp_base_get_prefix): New.
* common.opt (fdiagnostics-show-caret): New option.
* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
* lib/prune.exp: Add -fno-diagnostics-show-caret.


caret-diagnostics-20120409.diff
Description: Binary data


Re: [PATCH] Caret diagnostics

2012-04-09 Thread Jason Merrill

On 04/09/2012 04:01 PM, Manuel López-Ibáñez wrote:

* It uses  the default cutoff as max_width, whatever it is (as
controlled by -fmessage-length).
* It uses the pretty-printer. The text cannot (should not) wrap
because we still print only max_width chars at most.


Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still 
want to use COLUMNS to limit how much of the source we print.


Jason


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Manuel López-Ibáñez
On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 I'll be happy to change it to whatever is more understandable. I think
 in CSS is called padding.


 That wouldn't be any clearer; my confusion was that I thought you were
 padding the right side of the source line, but you aren't; you are only
 making sure that the caret isn't too close to the edge of the terminal.  No
 need to change anything here.

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.

That code is useless. It is overridden by cxx_initialize_diagnostics
(line 2429). It is not strange that nobody noticed this because the
pretty-printer code is hard to understand and impossible to debug. I'd
rather not touch it if I can avoid it.

I am not even sure if that is the correct way to set the line-cut-off
in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
knows what is the correct way?

Also, using COLUMNS to initialize pp's line-cut-off will change the
current default (which is unlimited despite what invoke.texi says).

I can add locus_max_width to diagnostic_context, and use
getenv_columns to initialize it once. Would that be ok?

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Manuel López-Ibáñez
On 8 April 2012 17:14, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sun, Apr 8, 2012 at 7:06 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 I'll be happy to change it to whatever is more understandable. I think
 in CSS is called padding.


 That wouldn't be any clearer; my confusion was that I thought you were
 padding the right side of the source line, but you aren't; you are only
 making sure that the caret isn't too close to the edge of the terminal.  No
 need to change anything here.

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.

 That code is useless.

 I do not understand what you by that.  Could you elaborate?
 c-family/ChangeLog says it was split out of c_common_init_options.

I mean it has no effect because cxx_initialize_diagnostics overrides
it before returning. Step in that function and you will see
diagnostic_line_cutoff (context) change from 0 to 80 to 0.

 It is overridden by cxx_initialize_diagnostics
 (line 2429). It is not strange that nobody noticed this because the
 pretty-printer code is hard to understand and impossible to debug.

 I find the syllogism a bit specious.

Sorry,  I didn't express myself in the best way out of frustration. I
mean that the pretty-printer code is quite complex since there are
many interacting parts, it emulates inheritance in plain C, and there
is no encapsulation, so variables are sometimes modified directly and
other times by using various functions. The abundant use of
preprocessor macros as accessors and for implementing inheritance
makes quite hard to debug the code in gdb. But this is a well-known
problem of the whole GCC codebase. Probably, there is no better way to
implement the same functionality in plain C, or perhaps gdb is not
powerful enough, or perhaps I am using it incorrectly. In any case, I
am having a very hard time following how the various pretty-printers
are initialized.

  I'd rather not touch it if I can avoid it.

 I am not even sure if that is the correct way to set the line-cut-off
 in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
 knows what is the correct way?

 The documentation of pp_base_set_line_maximum_length says:

 /* Sets the number of maximum characters per line PRETTY-PRINTER can
   output in line-wrapping mode.  A LENGTH value 0 suppresses
   line-wrapping.  */

So when is it appropriate to use

diagnostic_line_cutoff (context) = 80;

like in the above code?

 Also, using COLUMNS to initialize pp's line-cut-off will change the
 current default (which is unlimited despite what invoke.texi says).

 I can add locus_max_width to diagnostic_context, and use
 getenv_columns to initialize it once. Would that be ok?

 I am not sure how its semantics would differ from pp_line_cutoff
 but if you are going to disable line-wrapping mode (which this
 effectively does), then the structure pp_wrapping_mode_t is
 where to place this kind of information.

Actually, what Jason is proposing is to initialize line_cutoff to
getevn(COLUMNS) if present or to unlimited if not, and then use that
for the caret diagnostic. I see two problems with this:

* We change the current default, which is no line-wrapping ever.

* Where should the default be set? By each FE? by diagnostic.c? by
pretty-printer.c?

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Manuel López-Ibáñez
On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.


To focus on the issue at hand, and independently of whether any bugs
exist or not, and whether I am capable or not to fix them. What you
are proposing is to change the current default of no line-wrapping to
line-wrap at getenv(COLUMNS), isn't it? And then, to use the same
value for the max_width in the function printing the caret, is that
correct?

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Gabriel Dos Reis
On Sun, Apr 8, 2012 at 11:10 AM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 8 April 2012 17:14, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sun, Apr 8, 2012 at 7:06 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 I'll be happy to change it to whatever is more understandable. I think
 in CSS is called padding.


 That wouldn't be any clearer; my confusion was that I thought you were
 padding the right side of the source line, but you aren't; you are only
 making sure that the caret isn't too close to the edge of the terminal.  No
 need to change anything here.

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; 
 I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.

 That code is useless.

 I do not understand what you by that.  Could you elaborate?
 c-family/ChangeLog says it was split out of c_common_init_options.

 I mean it has no effect because cxx_initialize_diagnostics overrides
 it before returning. Step in that function and you will see
 diagnostic_line_cutoff (context) change from 0 to 80 to 0.

OK.


 It is overridden by cxx_initialize_diagnostics
 (line 2429). It is not strange that nobody noticed this because the
 pretty-printer code is hard to understand and impossible to debug.

 I find the syllogism a bit specious.

 Sorry,  I didn't express myself in the best way out of frustration. I
 mean that the pretty-printer code is quite complex since there are
 many interacting parts, it emulates inheritance in plain C, and there
 is no encapsulation, so variables are sometimes modified directly and
 other times by using various functions. The abundant use of
 preprocessor macros as accessors and for implementing inheritance
 makes quite hard to debug the code in gdb. But this is a well-known
 problem of the whole GCC codebase. Probably, there is no better way to
 implement the same functionality in plain C, or perhaps gdb is not
 powerful enough, or perhaps I am using it incorrectly. In any case, I
 am having a very hard time following how the various pretty-printers
 are initialized.

Many of the pretty printer macros were introduced to abstract away
direct access to the fields.  The fact that some fields are accessed
directly in client codes is a bug in client codes, not a feature of the
pretty printer interface.  Alas C does not provide a convenient access
control, so these access violations go undetected when they slip
through careful reviews.  GDB has support for macros but it true
that debugging macro-based interfaces require some gymnastics,
and this one is no exception.


  I'd rather not touch it if I can avoid it.

 I am not even sure if that is the correct way to set the line-cut-off
 in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
 knows what is the correct way?

 The documentation of pp_base_set_line_maximum_length says:

 /* Sets the number of maximum characters per line PRETTY-PRINTER can
   output in line-wrapping mode.  A LENGTH value 0 suppresses
   line-wrapping.  */

 So when is it appropriate to use

 diagnostic_line_cutoff (context) = 80;

 like in the above code?

First let me say this:  if you are doing diagnostics with caret, then you do
not want line wrapping, otherwise it gets a bit ugly.  So you need
a way to tell the diagnostic outputter that you do not want line wrap.

diagnostic_line_cutoff suggests the maximum number of characters that
the diagnostic outputter could send before emitting a newline to begin
line wrap.  0 suggests no line wrap.

So, I would not try to reuse that field -- otherwise, it can get messed up.
You suggested in your previous mail to use a distinct field to store the
width the terminal; I think that is a better option.


 Also, using COLUMNS to initialize pp's line-cut-off will change the
 current default (which is unlimited despite what invoke.texi says).

 I can add locus_max_width to diagnostic_context, and use
 getenv_columns to initialize it once. Would that be ok?

 I am not sure how its semantics would differ from pp_line_cutoff
 but if you are going to disable line-wrapping mode (which this
 effectively does), then the structure pp_wrapping_mode_t is
 where to place this kind of information.

 Actually, what Jason is proposing is to initialize line_cutoff to
 getevn(COLUMNS) if present or to unlimited if not, and then use that
 for the caret diagnostic. I see two problems with this:

 * We change the current default, which is no line-wrapping ever.

 * Where should the default be set? By each FE? by diagnostic.c? by
 pretty-printer.c?


I would suggest the front-ends that want it.  If all front ends want it
then it makes sense to set it once for all in diagnostic.c.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Gabriel Dos Reis
On Sun, Apr 8, 2012 at 11:13 AM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.


 To focus on the issue at hand, and independently of whether any bugs
 exist or not, and whether I am capable or not to fix them. What you
 are proposing is to change the current default of no line-wrapping to
 line-wrap at getenv(COLUMNS), isn't it?

yes.

  And then, to use the same
 value for the max_width in the function printing the caret, is that
 correct?

yes.


 Cheers,

 Manuel.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Manuel López-Ibáñez
On 8 April 2012 18:35, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sun, Apr 8, 2012 at 11:13 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.


 To focus on the issue at hand, and independently of whether any bugs
 exist or not, and whether I am capable or not to fix them. What you
 are proposing is to change the current default of no line-wrapping to
 line-wrap at getenv(COLUMNS), isn't it?

 yes.

OK, so to do this, I have to figure out where the pretty-printer is
initialized in each FE. For C++ there are two different places:

init_error() initializes cxx_pp
whereas cxx_initialize_diagnostics seems to initialize a different
pretty-printer for C++.

What is the difference between the two?

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Gabriel Dos Reis
On Sun, Apr 8, 2012 at 11:52 AM, Manuel López-Ibáñez
lopeziba...@gmail.com wrote:
 On 8 April 2012 18:35, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sun, Apr 8, 2012 at 11:13 AM, Manuel López-Ibáñez
 lopeziba...@gmail.com wrote:
 On 8 April 2012 06:09, Jason Merrill ja...@redhat.com wrote:
 On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:

 +getenv_columns (void)


 I had been thinking to check COLUMNS once at the beginning of compilation; 
 I
 don't think the value can change while the compiler is running since we
 don't respond to SIGWINCH.  And let's use this value in
 c_common_initialize_diagnostics, too.


 To focus on the issue at hand, and independently of whether any bugs
 exist or not, and whether I am capable or not to fix them. What you
 are proposing is to change the current default of no line-wrapping to
 line-wrap at getenv(COLUMNS), isn't it?

 yes.

 OK, so to do this, I have to figure out where the pretty-printer is
 initialized in each FE. For C++ there are two different places:

 init_error() initializes cxx_pp

global_dc is the global diagnostic context -- that is used by almost
every front-end (except Ada and Fortran?)

cxx_pp is the C++ front-end-specific global pretty printer:  this is where
we dump C++ ASTs as strings. It is mostly used only by the various
tree - string functions that are occasionally called from the debugger
or by the front-end for things like __PRETTY_FUNCTION__.  It is NOT
for diagnostic
purposes.  (it was never meant to survive so many releases.)

 whereas cxx_initialize_diagnostics seems to initialize a different
 pretty-printer for C++.

This is where we allocate a C++ pretty printer attached to the diagnostic
context passed in as a parameter.  It is this pretty printer that is used by
the diagnostic machinery.


 What is the difference between the two?

 Cheers,

 Manuel.


Re: [PATCH] Caret diagnostics

2012-04-08 Thread Jason Merrill

On 04/08/2012 12:13 PM, Manuel López-Ibáñez wrote:

To focus on the issue at hand, and independently of whether any bugs
exist or not, and whether I am capable or not to fix them. What you
are proposing is to change the current default of no line-wrapping to
line-wrap at getenv(COLUMNS), isn't it?


I was just proposing to change the 80 to the value of 
getenv(COLUMNS).  If we don't currently use the value set there, let's 
not change that in this patch.



And then, to use the same
value for the max_width in the function printing the caret, is that
correct?


Yes.

Jason


Re: [PATCH] Caret diagnostics

2012-04-07 Thread Manuel López-Ibáñez
On 7 April 2012 04:31, Jason Merrill ja...@redhat.com wrote:
 On 04/06/2012 06:30 PM, Manuel López-Ibáñez wrote:

 width if it's set; otherwise I would lean toward unlimited width. And I'm
 not sure why we need a right margin at all.


 The right margin is because:

[snip]

 Ah, I read margin and assumed it meant you were leaving blank space at the
 right side of the screen.  Now I understand.

I'll be happy to change it to whatever is more understandable. I think
in CSS is called padding.

New version attached. Bootstrapped and regression tested including obj-c++.

Cheers,

Manuel.

2012-04-05  Manuel López-Ibáñez  m...@gcc.gnu.org

PR 24985
libstdc++-v3/
* testsuite/lib/prune.exp: Handle caret.
libmudflap/
* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
* diagnostic.h (show_caret): Declare.
(diagnostic_show_locus): Declare.
* diagnostic.c (diagnostic_initialize): Initialize to false.
(diagnostic_show_locus): New.
(diagnostic_report_diagnostic): Call it.
(getenv_columns): New function.
* input.c (read_line): New.
(location_get_source_line): New.
* input.h (location_get_source_line): Declare.
* toplev.c (general_init): Initialize show_caret from options.
* dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
* opts.c (common_handle_option): Likewise.
* common.opt (fdiagnostics-show-caret): New option.
* testsuite/lib/prune.exp: Add -fno-diagnostics-show-caret.


caret-diagnostics-20120408.diff
Description: Binary data


Re: [PATCH] Caret diagnostics

2012-04-07 Thread Jason Merrill

On 04/07/2012 06:29 PM, Manuel López-Ibáñez wrote:

I'll be happy to change it to whatever is more understandable. I think
in CSS is called padding.


That wouldn't be any clearer; my confusion was that I thought you were 
padding the right side of the source line, but you aren't; you are only 
making sure that the caret isn't too close to the edge of the terminal. 
 No need to change anything here.



+getenv_columns (void)


I had been thinking to check COLUMNS once at the beginning of 
compilation; I don't think the value can change while the compiler is 
running since we don't respond to SIGWINCH.  And let's use this value in 
c_common_initialize_diagnostics, too.



+# Also it may trim a source line by mistake since it matches repeated 
lines.


Let's clarify this comment; because dejagnu gets the whole compiler 
output as a single string, .* in a test regexp can match across multiple 
lines, which with caret diagnostics is likely to mean that the beginning 
of the regexp matches the error message and the end of the regexp 
matches the regexp itself in the printed source line.


Jason


Re: [PATCH] Caret diagnostics

2012-04-06 Thread Mike Stump
On Apr 6, 2012, at 1:11 AM, Manuel López-Ibáñez wrote:
 A simple implementation of caret diagnostics.
 
 In the testsuite, pruning the caret output does not always work
 because of several known deficiencies of DejaGNU, thus in some places
 I disable the caret explicitly.
 
 Bootstrapped and regression tested on x86_64-unknown-gnu-linux with
 enable-languages=all,ada and -m32/-m64.
 
 OK to commit?

Could someone take Objective-C++ for a spin?  It builds and runs pretty 
quickly, non-bootstrap is fine.

The testsuite changes are fine, though, it feels like you're missing the main 
prune routine?

Nice work, love to see it go in.


Re: [PATCH] Caret diagnostics

2012-04-06 Thread Manuel López-Ibáñez
On 6 April 2012 15:42, Mike Stump mikest...@comcast.net wrote:
 On Apr 6, 2012, at 1:11 AM, Manuel López-Ibáñez wrote:
 A simple implementation of caret diagnostics.

 In the testsuite, pruning the caret output does not always work
 because of several known deficiencies of DejaGNU, thus in some places
 I disable the caret explicitly.

 Bootstrapped and regression tested on x86_64-unknown-gnu-linux with
 enable-languages=all,ada and -m32/-m64.

 OK to commit?

 Could someone take Objective-C++ for a spin?  It builds and runs pretty 
 quickly, non-bootstrap is fine.

 The testsuite changes are fine, though, it feels like you're missing the main 
 prune routine?

I didn't change the prune routine because it doesn't work reliably
(because of DejaGNU problems PR12096 and PR30612 and others mentioned
in PR24985). That is why I set -fno-diagnostics-show-caret there.
Other places that have their own prune routines didn't show any
problems, so I prune the output instead of adding
-fno-diagnostics-show-caret to every command.

 Nice work, love to see it go in.

Thanks,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-06 Thread Jason Merrill

On 04/06/2012 04:11 AM, Manuel López-Ibáñez wrote:

+++ gcc/testsuite/gcc.dg/torture/tls/tls.exp(working copy)
@@ -48,10 +48,10 @@ dg-init
 torture-init
 set-torture-options $TLS_TORTURE_OPTIONS {{}} $LTO_TORTURE_OPTIONS

 # Main loop.
 gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
-$DEFAULT_CFLAGS
+-fno-diagnostics-show-caret $DEFAULT_CFLAGS


I don't think this is needed since you're already adding it to 
TEST_ALWAYS_FLAGS.  The tests pass without this for me.



+  int max_width = 80, right_margin = 10;


Let's not hardcode these numbers.  We should use getenv(COLUMNS) for 
the width if it's set; otherwise I would lean toward unlimited width. 
And I'm not sure why we need a right margin at all.



+  s = expand_location(diagnostic-location);
+  line = location_get_source_line (s);


Why not expand the location inside location_get_source_line?

Jason


Re: [PATCH] Caret diagnostics

2012-04-06 Thread Manuel López-Ibáñez
On 7 April 2012 00:04, Jason Merrill ja...@redhat.com wrote:
 On 04/06/2012 04:11 AM, Manuel López-Ibáńez wrote:

 +++ gcc/testsuite/gcc.dg/torture/tls/tls.exp    (working copy)
 @@ -48,10 +48,10 @@ dg-init
  torture-init
  set-torture-options $TLS_TORTURE_OPTIONS {{}} $LTO_TORTURE_OPTIONS

  # Main loop.
  gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
 -        $DEFAULT_CFLAGS
 +        -fno-diagnostics-show-caret $DEFAULT_CFLAGS


 I don't think this is needed since you're already adding it to
 TEST_ALWAYS_FLAGS.  The tests pass without this for me.


Good. I will try without, but the output does not show the flag two
times, which make me think that tls.exp is not using
TEST_ALWAYS_FLAGS. But maybe I was looking at the wrong place.

 +  int max_width = 80, right_margin = 10;


 Let's not hardcode these numbers.  We should use getenv(COLUMNS) for the

Fine, I'll try that.

 width if it's set; otherwise I would lean toward unlimited width. And I'm
 not sure why we need a right margin at all.

The right margin is because:

* We don't want to do wrapping. Otherwise the caret looks strange. So
either we cut long lines or we leave the maximum width unlimited.
* If we cut long lines, and the line is too long and the caret is too
close to the right margin, it is better to cut the line at the
beginning to show the part pointed by the caret within the maximum
width, with some margin to the right. That is, we don't want to show:

 very_very_long_line with something w
 ^
We want to show:

 long_line with something wrong on it
  ^


 +  s = expand_location(diagnostic-location);
 +  line = location_get_source_line (s);


 Why not expand the location inside location_get_source_line?

Well, we have to expand outside anyway, because I need the column
info, so it seemed a waste to expand twice. Do you have a strong
preference for expanding twice? I don't care so much either way.

Cheers,

Manuel.


Re: [PATCH] Caret diagnostics

2012-04-06 Thread Jason Merrill

On 04/06/2012 06:30 PM, Manuel López-Ibáñez wrote:

width if it's set; otherwise I would lean toward unlimited width. And I'm
not sure why we need a right margin at all.


The right margin is because:

[snip]

Ah, I read margin and assumed it meant you were leaving blank space at 
the right side of the screen.  Now I understand.



Well, we have to expand outside anyway, because I need the column
info, so it seemed a waste to expand twice. Do you have a strong
preference for expanding twice? I don't care so much either way.


No, I guess it's fine this way.

Jason