Re: [PATCH] Caret diagnostics
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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