|

»

»

Gnu - Bison - Patches

[PATCH] Keep sub-messages aligned. Fix strings for translation.

View: New views

6 Messages — Rating Filter: Alert me

	[PATCH] Keep sub-messages aligned. Fix strings for translation. by Alex Rozenman :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Hi Akim, Hi Joel, All, I installed the following patch (attached) on "master" and "branch-2.5" branches. The patch fixes some translation issues, introduces an alignment mechanism for sub-messages and adds an announcement to NEWS. I am feeling now, that the coding phase for this feature is nearly completed - I am not seeing any open issues any more. I have a question about documentation. There are two basic options - to write a new chapter about this feature and keep all the rest untouched, or (and) to update all the text, mixing examples of old style with new ones. Another question: who can assist me with the documentation ? :) -- Best regards, Alex Rozenman (rozenman@...). commit 66381412d93f6a54e4d8e7e90b89149a9bca4945 Author: Alex Rozenman <rozenman@...> Date: Sat Sep 19 12:59:33 2009 +0300 Keep sub-messages aligned. Fix strings for translation. * src/location.h: (location_print): Add return value. * src/location.c: (location_print): Return number of printed characters. * src/complain.h: Two new functions (complain_at_indent, warn_at_indent). * src/complain.cpp: Implement the alignment mechanism. Add new static variable (indent_ptr). Use and update it (error_message, complain_at_indent, warn_at_indent). * src/scan-code.l: Fix strings for translations. Use new _indent functions (parse_ref, show_sub_messages). NEWS (2.5): Add an announcement about named references. diff --git a/ChangeLog b/ChangeLog index 2811304..ca92292 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,18 @@ +2009-09-19 Alex Rozenman <rozenman@...> + + Keep sub-messages aligned. Fix strings for translation. + * src/location.h: (location_print): Add return value. + * src/location.c: (location_print): Return number of printed + characters. + * src/complain.h: Two new functions (complain_at_indent, + warn_at_indent). + * src/complain.cpp: Implement the alignment mechanism. Add new + static variable (indent_ptr). Use and update it (error_message, + complain_at_indent, warn_at_indent). + * src/scan-code.l: Fix strings for translations. Use new _indent + functions (parse_ref, show_sub_messages). + NEWS (2.5): Add an announcement about named references. + 2009-09-17 Akim Demaille <demaille@...> doc: fixes. diff --git a/NEWS b/NEWS index 41ab73d..c80c40f 100644 --- a/NEWS +++ b/NEWS @@ -53,6 +53,31 @@ Bison News * Changes in version 2.5 (????-??-??): + Named References Support + + Historically, Yacc and Bison have supported positional references + ($n, $$) to allow access to symbol values from inside of semantic + actions code. + + Starting from this version, Bison can also accept named references. + When no ambiguity is possible, original symbol names may be used + as named references: + + if_stmt : 'if' cond_expr 'then' then_stmt ';' + { $if_stmt = mk_if_stmt($cond_expr, $then_stmt); } + + In the more common case, explicit names may be declared: + + stmt[res] : 'if' expr[cond] 'then' stmt[then] 'else' stmt[else] ';' + { $res = mk_if_stmt($cond, $then, $else); } + + Location information is also accessible using @name syntax. When + accessing symbol names containing dots or dashes, explicit bracketing + ($[sym.1]) must be used. + + These features are experimental in this version. More user feedback + will help to stabilize them. + IELR(1) and Canonical LR(1) Support IELR(1) is a minimal LR(1) parser table generation algorithm. That diff --git a/src/complain.c b/src/complain.c index 4cc35c8..7bb22de 100644 --- a/src/complain.c +++ b/src/complain.c @@ -29,6 +29,7 @@ #include "getargs.h" bool complaint_issued; +static unsigned indent_ptr = 0; @@ -46,11 +47,22 @@ error_message (location loc, const char prefix, const char message, va_list args) { + unsigned pos = 0; + if (loc) - location_print (stderr, loc); + pos += location_print (stderr, loc); else - fputs (current_file ? current_file : program_name, stderr); - fputs (": ", stderr); + pos += fprintf(stderr, "%s", current_file ? current_file : program_name); + pos += fprintf(stderr, ": "); + + if (indent_ptr) + { + if (!indent_ptr) + indent_ptr = pos; + else if (indent_ptr > pos) + fprintf (stderr, "%s", indent_ptr - pos, ""); + indent_ptr = 0; + } if (prefix) fprintf (stderr, "%s: ", prefix); @@ -94,6 +106,15 @@ warn_at (location loc, const char message, ...) } void +warn_at_indent (location loc, unsigned indent, + const char message, ...) +{ + set_warning_issued (); + indent_ptr = indent; + ERROR_MESSAGE (&loc, _("warning"), message); +} + +void warn (const char message, ...) { set_warning_issued (); @@ -113,6 +134,15 @@ complain_at (location loc, const char message, ...) } void +complain_at_indent (location loc, unsigned indent, + const char message, ...) +{ + indent_ptr = indent; + ERROR_MESSAGE (&loc, NULL, message); + complaint_issued = true; +} + +void complain (const char message, ...) { ERROR_MESSAGE (NULL, NULL, message); diff --git a/src/complain.h b/src/complain.h index 89cdd91..fe4ecf7 100644 --- a/src/complain.h +++ b/src/complain.h @@ -31,6 +31,13 @@ void warn (char const format, ...) void warn_at (location loc, char const format, ...) __attribute__ ((__format__ (__printf__, 2, 3))); +/ Generate a message aligned by an indent. + When indent == 0, assign message's indent to indent, + When indent > 0, align the message by indent value. / +void warn_at_indent (location loc, unsigned indent, + char const format, ...) + __attribute__ ((__format__ (__printf__, 3, 4))); + /* An error, but we continue and die later. / void complain (char const format, ...) @@ -39,6 +46,13 @@ void complain (char const format, ...) void complain_at (location loc, char const format, ...) __attribute__ ((__format__ (__printf__, 2, 3))); +/* Generate a message aligned by an indent. + When indent == 0, assign message's indent to indent, + When indent > 0, align the message by indent value. / +void complain_at_indent (location loc, unsigned indent, + char const format, ...) + __attribute__ ((__format__ (__printf__, 3, 4))); + /* An incompatibility with POSIX Yacc: mapped either to warn* or complain* depending on yacc_flag. / diff --git a/src/location.c b/src/location.c index 4cbfd8d..e287aee 100644 --- a/src/location.c +++ b/src/location.c @@ -98,40 +98,44 @@ location_compute (location loc, boundary cur, char const token, size_t size) /* Output to OUT the location LOC. Warning: it uses quotearg's slot 3. / -void +unsigned location_print (FILE out, location loc) { + unsigned res = 0; int end_col = 0 != loc.end.column ? loc.end.column - 1 : 0; - fprintf (out, "%s", - quotearg_n_style (3, escape_quoting_style, loc.start.file)); + res += fprintf (out, "%s", + quotearg_n_style (3, escape_quoting_style, loc.start.file)); if (0 <= loc.start.line) { - fprintf(out, ":%d", loc.start.line); + res += fprintf(out, ":%d", loc.start.line); if (0 <= loc.start.column) - fprintf (out, ".%d", loc.start.column); + res += fprintf (out, ".%d", loc.start.column); } if (loc.start.file != loc.end.file) { - fprintf (out, "-%s", - quotearg_n_style (3, escape_quoting_style, loc.end.file)); + res += fprintf (out, "-%s", + quotearg_n_style (3, escape_quoting_style, + loc.end.file)); if (0 <= loc.end.line) { - fprintf(out, ":%d", loc.end.line); + res += fprintf(out, ":%d", loc.end.line); if (0 <= end_col) - fprintf (out, ".%d", end_col); + res += fprintf (out, ".%d", end_col); } } else if (0 <= loc.end.line) { if (loc.start.line < loc.end.line) { - fprintf (out, "-%d", loc.end.line); + res += fprintf (out, "-%d", loc.end.line); if (0 <= end_col) - fprintf (out, ".%d", end_col); + res += fprintf (out, ".%d", end_col); } else if (0 <= end_col && loc.start.column < end_col) - fprintf (out, "-%d", end_col); + res += fprintf (out, "-%d", end_col); } + + return res; } void diff --git a/src/location.h b/src/location.h index ec48164..95c5a07 100644 --- a/src/location.h +++ b/src/location.h @@ -98,7 +98,9 @@ extern location const empty_location; void location_compute (location loc, boundary cur, char const token, size_t size); -void location_print (FILE out, location loc); +/* Print location to file. Return number of actually printed + characters. / +unsigned location_print (FILE out, location loc); /* Return -1, 0, 1, depending whether a is before, equal, or after b. / diff --git a/src/scan-code.l b/src/scan-code.l index b207716..56cd782 100644 --- a/src/scan-code.l +++ b/src/scan-code.l @@ -410,7 +410,8 @@ get_at_spec(unsigned symbol_index) static void show_sub_messages (const char cp, bool explicit_bracketing, - int midrule_rhs_index, char dollar_or_at, bool is_warning) + int midrule_rhs_index, char dollar_or_at, + bool is_warning, unsigned indent) { unsigned i; @@ -422,11 +423,11 @@ show_sub_messages (const char* cp, bool explicit_bracketing, if (var->err == 0) { if (is_warning) - warn_at (var->loc, _(" refers to: %c%s at %s"), - dollar_or_at, var->id, at_spec); + warn_at_indent (var->loc, &indent, _("refers to: %c%s at %s"), + dollar_or_at, var->id, at_spec); else - complain_at (var->loc, _(" refers to: %c%s at %s"), - dollar_or_at, var->id, at_spec); + complain_at_indent (var->loc, &indent, _("refers to: %c%s at %s"), + dollar_or_at, var->id, at_spec); } else { @@ -441,7 +442,7 @@ show_sub_messages (const char* cp, bool explicit_bracketing, /* Create the explanation message. / obstack_init (&msg_buf); - obstack_fgrow1 (&msg_buf, " possibly meant: %c", dollar_or_at); + obstack_fgrow1 (&msg_buf, _("possibly meant: %c"), dollar_or_at); if (contains_dot_or_dash (id)) obstack_fgrow1 (&msg_buf, "[%s]", id); else @@ -450,7 +451,7 @@ show_sub_messages (const char cp, bool explicit_bracketing, if (var->err & VARIANT_HIDDEN) { - obstack_fgrow1 (&msg_buf, ", hiding %c", dollar_or_at); + obstack_fgrow1 (&msg_buf, _(", hiding %c"), dollar_or_at); if (contains_dot_or_dash (var->id)) obstack_fgrow1 (&msg_buf, "[%s]", var->id); else @@ -458,17 +459,22 @@ show_sub_messages (const char* cp, bool explicit_bracketing, obstack_sgrow (&msg_buf, tail); } - obstack_fgrow1 (&msg_buf, " at %s", at_spec); + obstack_fgrow1 (&msg_buf, _(" at %s"), at_spec); if (var->err & VARIANT_NOT_VISIBLE_FROM_MIDRULE) - obstack_fgrow1 (&msg_buf, ", cannot be accessed from " - "mid-rule action at $%d", midrule_rhs_index); + { + const char format = + _(", cannot be accessed from mid-rule action at $%d"); + obstack_fgrow1 (&msg_buf, format, midrule_rhs_index); + } obstack_1grow (&msg_buf, '\0'); if (is_warning) - warn_at (id_loc, _("%s"), (char ) obstack_finish (&msg_buf)); + warn_at_indent (id_loc, &indent, "%s", + (char ) obstack_finish (&msg_buf)); else - complain_at (id_loc, _("%s"), (char ) obstack_finish (&msg_buf)); + complain_at_indent (id_loc, &indent, "%s", + (char ) obstack_finish (&msg_buf)); obstack_free (&msg_buf, 0); } } @@ -482,6 +488,9 @@ show_sub_messages (const char cp, bool explicit_bracketing, points to LHS ($$) of the current rule or midrule. / #define LHS_REF (INT_MIN + 1) +/ Sub-messages indent. / +#define SUB_INDENT (4) + / Parse named or positional reference. In case of positional references, can return negative values for $-n "deep" stack accesses. / @@ -594,29 +603,40 @@ parse_ref (char cp, symbol_list rule, int rule_length, { unsigned len = (explicit_bracketing \|\| !ref_tail_fields) ? cp_end - cp : ref_tail_fields - cp; - const char message = "symbol not found in production"; + unsigned indent = 0; - complain_at (text_loc, _("invalid reference: %s"), quote (text)); + complain_at_indent (text_loc, &indent, _("invalid reference: %s"), + quote (text)); + indent += SUB_INDENT; if (midrule_rhs_index) - complain_at (rule->location, _(" %s before $%d: %.s"), - message, midrule_rhs_index, len, cp); + { + const char format = + _("symbol not found in production before $%d: %.s"); + complain_at_indent (rule->location, &indent, format, + midrule_rhs_index, len, cp); + } else - complain_at (rule->location, _(" %s: %.s"), - message, len, cp); + { + const char format = + _("symbol not found in production: %.s"); + complain_at_indent (rule->location, &indent, format, + len, cp); + } if (variant_count > 0) show_sub_messages (cp, explicit_bracketing, midrule_rhs_index, - dollar_or_at, false); + dollar_or_at, false, indent); return INVALID_REF; } case 1: { + unsigned indent = 0; if (variant_count > 1) { - warn_at (text_loc, _("misleading reference: %s"), - quote (text)); + warn_at_indent (text_loc, &indent, _("misleading reference: %s"), + quote (text)); show_sub_messages (cp, explicit_bracketing, midrule_rhs_index, - dollar_or_at, true); + dollar_or_at, true, indent + SUB_INDENT); } { unsigned symbol_index = @@ -626,11 +646,14 @@ parse_ref (char cp, symbol_list rule, int rule_length, } case 2: default: - complain_at (text_loc, _("ambiguous reference: %s"), - quote (text)); - show_sub_messages (cp, explicit_bracketing, midrule_rhs_index, - dollar_or_at, false); - return INVALID_REF; + { + unsigned indent = 0; + complain_at_indent (text_loc, &indent, _("ambiguous reference: %s"), + quote (text)); + show_sub_messages (cp, explicit_bracketing, midrule_rhs_index, + dollar_or_at, false, indent + SUB_INDENT); + return INVALID_REF; + } } /* Not reachable. */ diff --git a/tests/named-refs.at b/tests/named-refs.at index 9ab63e4..4a256a1 100644 --- a/tests/named-refs.at +++ b/tests/named-refs.at @@ -256,16 +256,16 @@ exp: AT_BISON_CHECK([-o test.c test.y], 1, [], [[test.y:50.51-60: invalid reference: `$<ival>lo9' -test.y:50.3-68: symbol not found in production: lo9 +test.y:50.3-68: symbol not found in production: lo9 test.y:51.51-60: warning: misleading reference: `$<ival>exp' -test.y:42.1-3: warning: refers to: $exp at $$ -test.y:51.7: warning: possibly meant: $x, hiding $exp at $1 -test.y:51.41: warning: possibly meant: $r, hiding $exp at $4 +test.y:42.1-3: warning: refers to: $exp at $$ +test.y:51.7: warning: possibly meant: $x, hiding $exp at $1 +test.y:51.41: warning: possibly meant: $r, hiding $exp at $4 test.y:52.51-52: $l of `exp' has no declared type test.y:55.46-49: invalid reference: `$r12' -test.y:55.3-53: symbol not found in production: r12 +test.y:55.3-53: symbol not found in production: r12 test.y:56.29-33: invalid reference: `$expo' -test.y:56.3-46: symbol not found in production: expo +test.y:56.3-46: symbol not found in production: expo ]]) AT_CLEANUP @@ -281,8 +281,8 @@ foo.bar: '2' ]]) AT_BISON_CHECK([-o test.c test.y], 0, [], [[test.y:11.22-29: warning: misleading reference: `$foo.bar' -test.y:11.8-10: warning: refers to: $foo at $1 -test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 +test.y:11.8-10: warning: refers to: $foo at $1 +test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 ]]) AT_CLEANUP @@ -357,44 +357,44 @@ factor: '(' expr ')' { $$ = $2; } ]]) AT_BISON_CHECK([-o test.c test.y], 1, [], [[test.y:24.36-41: invalid reference: `$cond1' -test.y:23.11-24.62: symbol not found in production: cond1 +test.y:23.11-24.62: symbol not found in production: cond1 test.y:26.43-53: invalid reference: `$stmt.field' -test.y:25.11-26.60: symbol not found in production: stmt -test.y:25.35-38: possibly meant: $then.field, hiding $stmt.field at $4 +test.y:25.11-26.60: symbol not found in production: stmt +test.y:25.35-38: possibly meant: $then.field, hiding $stmt.field at $4 test.y:28.43-52: invalid reference: `$stmt.list' -test.y:27.11-28.59: symbol not found in production: stmt -test.y:27.30-38: possibly meant: $[stmt.list] at $4 +test.y:27.11-28.59: symbol not found in production: stmt +test.y:27.30-38: possibly meant: $[stmt.list] at $4 test.y:30.43-46: ambiguous reference: `$xyz' -test.y:29.35-37: refers to: $xyz at $4 -test.y:29.50-52: refers to: $xyz at $6 +test.y:29.35-37: refers to: $xyz at $4 +test.y:29.50-52: refers to: $xyz at $6 test.y:32.43-52: invalid reference: `$stmt.list' -test.y:31.11-32.63: symbol not found in production: stmt -test.y:31.40-43: possibly meant: $then, hiding $[stmt.list] at $4 -test.y:31.61-64: possibly meant: $else, hiding $[stmt.list] at $6 +test.y:31.11-32.63: symbol not found in production: stmt +test.y:31.40-43: possibly meant: $then, hiding $[stmt.list] at $4 +test.y:31.61-64: possibly meant: $else, hiding $[stmt.list] at $6 test.y:34.43-58: invalid reference: `$stmt.list.field' -test.y:33.11-34.69: symbol not found in production: stmt -test.y:33.40-43: possibly meant: $then.field, hiding $[stmt.list].field at $4 -test.y:33.61-64: possibly meant: $else.field, hiding $[stmt.list].field at $6 +test.y:33.11-34.69: symbol not found in production: stmt +test.y:33.40-43: possibly meant: $then.field, hiding $[stmt.list].field at $4 +test.y:33.61-64: possibly meant: $else.field, hiding $[stmt.list].field at $6 test.y:36.43-54: invalid reference: `$[stmt.list]' -test.y:35.11-36.71: symbol not found in production: stmt.list -test.y:35.40-43: possibly meant: $then, hiding $[stmt.list] at $4 -test.y:35.61-64: possibly meant: $else, hiding $[stmt.list] at $6 +test.y:35.11-36.71: symbol not found in production: stmt.list +test.y:35.40-43: possibly meant: $then, hiding $[stmt.list] at $4 +test.y:35.61-64: possibly meant: $else, hiding $[stmt.list] at $6 test.y:38.43-49: invalid reference: `$then.1' -test.y:37.11-38.60: symbol not found in production: then -test.y:37.40-45: possibly meant: $[then.1] at $4 +test.y:37.11-38.60: symbol not found in production: then +test.y:37.40-45: possibly meant: $[then.1] at $4 test.y:40.43-55: invalid reference: `$then.1.field' -test.y:39.11-40.66: symbol not found in production: then -test.y:39.40-45: possibly meant: $[then.1].field at $4 +test.y:39.11-40.66: symbol not found in production: then +test.y:39.40-45: possibly meant: $[then.1].field at $4 test.y:42.44-50: invalid reference: `$stmt.x' -test.y:41.12-42.57: symbol not found in production: stmt -test.y:41.36-41: possibly meant: $[stmt.x].x, hiding $stmt.x at $4 -test.y:41.36-41: possibly meant: $[stmt.x] at $4 +test.y:41.12-42.57: symbol not found in production: stmt +test.y:41.36-41: possibly meant: $[stmt.x].x, hiding $stmt.x at $4 +test.y:41.36-41: possibly meant: $[stmt.x] at $4 test.y:44.13-22: invalid reference: `$if-stmt-a' -test.y:43.12-44.59: symbol not found in production: if -test.y:43.1-9: possibly meant: $[if-stmt-a] at $$ +test.y:43.12-44.59: symbol not found in production: if +test.y:43.1-9: possibly meant: $[if-stmt-a] at $$ test.y:46.46-54: invalid reference: `$then-a.f' -test.y:45.12-46.65: symbol not found in production: then -test.y:45.41-46: possibly meant: $[then-a].f at $4 +test.y:45.12-46.65: symbol not found in production: then +test.y:45.41-46: possibly meant: $[then-a].f at $4 ]]) AT_CLEANUP @@ -528,36 +528,36 @@ sym_b : 'b'; ]]) AT_BISON_CHECK([-o test.c test.y], 1, [], [[test.y:13.8-17: invalid reference: `$sym.field' -test.y:12.1-13.21: symbol not found in production: sym +test.y:12.1-13.21: symbol not found in production: sym test.y:16.8-21: invalid reference: `$<aa>sym.field' -test.y:15.1-16.25: symbol not found in production: sym +test.y:15.1-16.25: symbol not found in production: sym test.y:19.8-19: invalid reference: `$[sym.field]' -test.y:18.1-19.23: symbol not found in production: sym.field +test.y:18.1-19.23: symbol not found in production: sym.field test.y:22.8-23: invalid reference: `$<aa>[sym.field]' -test.y:21.1-22.27: symbol not found in production: sym.field +test.y:21.1-22.27: symbol not found in production: sym.field test.y:25.8-11: invalid reference: `$sym' -test.y:24.1-25.15: symbol not found in production: sym +test.y:24.1-25.15: symbol not found in production: sym test.y:28.8-15: invalid reference: `$<aa>sym' -test.y:27.1-28.19: symbol not found in production: sym +test.y:27.1-28.19: symbol not found in production: sym test.y:31.8-13: invalid reference: `$[sym]' -test.y:30.1-33.21: symbol not found in production before $3: sym +test.y:30.1-33.21: symbol not found in production before $3: sym test.y:33.8-17: invalid reference: `$<aa>[sym]' -test.y:30.1-33.21: symbol not found in production: sym +test.y:30.1-33.21: symbol not found in production: sym test.y:37.8-17: invalid reference: `$sym-field' -test.y:36.1-37.21: symbol not found in production: sym +test.y:36.1-37.21: symbol not found in production: sym test.y:40.8-21: invalid reference: `$<aa>sym-field' -test.y:39.1-40.25: symbol not found in production: sym +test.y:39.1-40.25: symbol not found in production: sym test.y:43.8-19: invalid reference: `$[sym-field]' -test.y:42.1-43.23: symbol not found in production: sym-field +test.y:42.1-43.23: symbol not found in production: sym-field test.y:46.8-23: invalid reference: `$<aa>[sym-field]' -test.y:45.1-46.27: symbol not found in production: sym-field +test.y:45.1-46.27: symbol not found in production: sym-field test.y:49.8-11: invalid reference: `$sym' -test.y:48.1-49.15: symbol not found in production: sym +test.y:48.1-49.15: symbol not found in production: sym test.y:52.8-15: invalid reference: `$<aa>sym' -test.y:51.1-52.19: symbol not found in production: sym +test.y:51.1-52.19: symbol not found in production: sym test.y:55.8-13: invalid reference: `$[sym]' -test.y:54.1-57.21: symbol not found in production before $3: sym +test.y:54.1-57.21: symbol not found in production before $3: sym test.y:57.8-17: invalid reference: `$<aa>[sym]' -test.y:54.1-57.21: symbol not found in production: sym +test.y:54.1-57.21: symbol not found in production: sym ]]) AT_CLEANUP
	Re: [PATCH] Keep sub-messages aligned. Fix strings for translation. by Joel E. Denny-4 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Hi Alex. On Sat, 19 Sep 2009, Alex Rozenman wrote: > I installed the following patch (attached) on "master" and > "branch-2.5" branches. The patch fixes some translation issues, > introduces an alignment mechanism for sub-messages and adds an > announcement to NEWS. Thanks. > I have a question about documentation. There are two basic options - > to write a new chapter about this feature and keep all the rest > untouched, or (and) to update all the text, mixing examples of old > style with new ones. It will probably be easiest to write and review if you start with an isolated section. We can integrate into other sections later. This should also make life easier in the future if the interface evolves before we remove the experimental status. > Another question: who can assist me with the documentation ? :) Submit patches, and we'll review as usual. I don't have much time to write anything myself. You could grab some text from previous emails we've exchanged if that helps. > +2009-09-19 Alex Rozenman <rozenman@...> > + > + Keep sub-messages aligned. Fix strings for translation. > + * src/location.h: (location_print): Add return value. > + * src/location.c: (location_print): Return number of printed > + characters. > + * src/complain.h: Two new functions (complain_at_indent, > + warn_at_indent). > + * src/complain.cpp: Implement the alignment mechanism. Add new > + static variable (indent_ptr). Use and update it (error_message, > + complain_at_indent, warn_at_indent). > + * src/scan-code.l: Fix strings for translations. Use new _indent > + functions (parse_ref, show_sub_messages). > + NEWS (2.5): Add an announcement about named references. Please try to follow the format you see in other ChangeLog entries. Specifically, put top-level constructs in parentheses after the file names, and don't add extra colons. So, the above should have been written more like: 2009-09-19 Alex Rozenman <rozenman@...> Keep sub-messages aligned. Fix strings for translation. * src/location.h (location_print): Add return value. * src/location.c (location_print): Return number of printed characters. * src/complain.h (complain_at_indent, warn_at_indent): Prototype new functions. * src/complain.c (indent_ptr): New static variable. (error_message, complain_at_indent, warn_at_indent): Use indent_ptr to implement the alignment mechanism. * src/scan-code.l (parse_ref, show_sub_messages): Fix strings for translations. Use new _indent functions. NEWS (2.5): Add an announcement about named references. Also, I see no mention of tests/named-refs.at, but that file changed. I prefer it if file names in ChangeLog entries are sorted in the same order that `git diff' or `git log -p' sorts them. This makes it easier to review the patch. Occasionally, I find it easier to write the ChangeLog entry if I change the order slightly, but I don't see a motivation here. > diff --git a/NEWS b/NEWS > + Location information is also accessible using @name syntax. When > + accessing symbol names containing dots or dashes, explicit bracketing > + ($[sym.1]) must be used. > + > + These features are experimental in this version. More user feedback > + will help to stabilize them. Throughout Bison documentation (including the ChangeLog and source code comments), we prefer two spaces after each period. > diff --git a/src/complain.c b/src/complain.c > @@ -46,11 +47,22 @@ error_message (location loc, > const char prefix, > const char message, va_list args) > { > + unsigned pos = 0; > + > if (loc) > - location_print (stderr, loc); > + pos += location_print (stderr, loc); > else > - fputs (current_file ? current_file : program_name, stderr); > - fputs (": ", stderr); > + pos += fprintf(stderr, "%s", current_file ? current_file : program_name); > + pos += fprintf(stderr, ": "); > + > + if (indent_ptr) > + { > + if (!indent_ptr) > + indent_ptr = pos; > + else if (indent_ptr > pos) > + fprintf (stderr, "%s", indent_ptr - pos, ""); > + indent_ptr = 0; > + } > > if (prefix) > fprintf (stderr, "%s: ", prefix); I would have moved the indentation after the prefix. That is, I think it looks a little strange that "warning" is indented along with the submessages. You can see examples of this in your test suite changes below. > void > +warn_at_indent (location loc, unsigned indent, > + const char message, ...) > +{ > + set_warning_issued (); > + indent_ptr = indent; > + ERROR_MESSAGE (&loc, _("warning"), message); > +} Rather than reimplement warn_at, why not invoke it after setting indent_ptr? It's a fairly small reimplementation, but that could evolve, so let's start clean. > void > +complain_at_indent (location loc, unsigned indent, > + const char message, ...) > +{ > + indent_ptr = indent; > + ERROR_MESSAGE (&loc, NULL, message); > + complaint_issued = true; > +} Likewise for complain_at. > +/* Generate a message aligned by an indent. > + When indent == 0, assign message's indent to indent, > + When indent > 0, align the message by indent value. / > +void warn_at_indent (location loc, unsigned indent, > + char const format, ...) > + __attribute__ ((__format__ (__printf__, 3, 4))); > + Rather than forcing the caller to deal with the indentation variable, it seems like it would be cleaner if warn_at automatically set the indentation level in a static global variable. Any subsequent warn_at_indent invocation would automatically use that indentation level. And the next warn_at invocation would reset it, etc. > +/ Generate a message aligned by an indent. > + When indent == 0, assign message's indent to indent, > + When indent > 0, align the message by indent value. / > +void complain_at_indent (location loc, unsigned indent, > + char const format, ...) > + __attribute__ ((__format__ (__printf__, 3, 4))); > + Likewise for complain_at and complain_at_indent. > +/ Sub-messages indent. */ > +#define SUB_INDENT (4) > + I think the number of spaces per indentation level should be consistent throughout Bison error messages, so that's more motivation to move the indentation implementation fully into complain.c as described above. > diff --git a/tests/named-refs.at b/tests/named-refs.at > AT_BISON_CHECK([-o test.c test.y], 0, [], > [[test.y:11.22-29: warning: misleading reference: `$foo.bar' > -test.y:11.8-10: warning: refers to: $foo at $1 > -test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 > +test.y:11.8-10: warning: refers to: $foo at $1 > +test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 > ]]) That now looks like: test.y:11.22-29: warning: misleading reference: `$foo.bar' test.y:11.8-10: warning: refers to: $foo at $1 test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 As I mentioned earlier, I would prefer: test.y:11.22-29: warning: misleading reference: `$foo.bar' test.y:11.8-10: warning: refers to: $foo at $1 test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 Or, ideally, if it won't somehow cause trouble for tools like Emacs: test.y:11.22-29: warning: misleading reference: `$foo.bar' test.y:11.8-10: warning: refers to: $foo at $1 test.y:11.12-18: warning: possibly meant: $[foo.bar] at $2 My guess is that it will be easy to adjust the code to make this happen once the implementation details of tracking the indentation level are fully hidden within complain.c.
	Re: [PATCH] Keep sub-messages aligned. Fix strings for translation. by Joel E. Denny-4 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Tue, 29 Sep 2009, Joel E. Denny wrote: > On Sat, 19 Sep 2009, Alex Rozenman wrote: > > +/* Generate a message aligned by an indent. > > + When indent == 0, assign message's indent to indent, > > + When indent > 0, align the message by indent value. / > > +void warn_at_indent (location loc, unsigned indent, > > + char const *format, ...) > > + __attribute__ ((__format__ (__printf__, 3, 4))); > > + > > Rather than forcing the caller to deal with the indentation variable, it > seems like it would be cleaner if warn_at automatically set the > indentation level in a static global variable. Any subsequent > warn_at_indent invocation would automatically use that indentation level. > And the next warn_at invocation would reset it, etc. While I still think that would be better than the current implementation, there's of course a bigger problem where neither implementation is good. It appears that named reference message blocks have the nice property that the location printed on the first line is not usually much shorter than the locations printed on subsequent lines. When that's not the case, neither your implementation nor my proposed one would indent properly. If we try to use indentation in error messages for other parts of Bison where that nice property might rarely hold, my proposed implementation is going to be hopeless. At least with your implementation we can adjust the caller to try to compute the necessary indentation all up front and store it in the indentation variable before any invocation of complain_at or complaint_at_indent. But that sounds painful to have to do every time. Instead, all indentation work ought to be performed within complain.c. Here's an example of how a better interface might be used: // Clear any previously buffered message, // and set indentation level to 0. error_start (); complain_at_save (loc0, msg0); // msg0 saved at indentation level 0. error_indent (); complain_at_save (loc1, msg1); // msg1 saved at indentation level 1. complain_at_save (loc2, msg2); // msg2 saved at indentation level 1. complain_at_save (loc3, msg3); // msg3 saved at indentation level 1. error_indent (); complain_at_save (loc4, msg4); // msg4 saved at indentation level 2. complain_at_save (loc5, msg5); // msg5 saved at indentation level 2. error_unindent (); complain_at_save (loc6, msg6); // msg6 saved at indentation level 1. error_end (); // Entire message block is printed. ... // msg9 prints immediately at level 0 ignoring whatever is currently // saved. For example, there may have been some sort of failure while // building a previous error message block. complain_at (loc9, msg9); Implementing the message buffering will be a little tedious, but if we want automatic indentation, I think something like this is the robust way to do it.
	Re: [PATCH] Keep sub-messages aligned. Fix strings for translation. by Alex Rozenman :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Hi Joel, Hi Akim, All > Please try to follow the format you see in other ChangeLog entries. > Specifically, put top-level constructs in parentheses after the file > names, and don't add extra colons. > > Throughout Bison documentation (including the ChangeLog and source code > comments), we prefer two spaces after each period. I pushed a small patch (attached) fixing some of minor issues described in your mail. Thank you for the explanations - your pedantry is awesome. It's installed on master/branch-2.5. I will write a separate mail about the main issue. > > Please try to follow the format you see in other ChangeLog entries. > Specifically, put top-level constructs in parentheses after the file > names, and don't add extra colons. >> diff --git a/NEWS b/NEWS > > Throughout Bison documentation (including the ChangeLog and source code > comments), we prefer two spaces after each period. -- Best regards, Alex Rozenman (rozenman@...). commit 5b1ff42379edbbf37b135d33aa5fb3c72da2dca1 Author: Alex Rozenman <rozenman@...> Date: Sat Oct 3 18:29:14 2009 +0200 Add additional space after periods in NEWS. * NEWS (2.5): here. diff --git a/ChangeLog b/ChangeLog index 1b0bb87..35b71ee 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2009-10-03 Alex Rozenman <rozenman@...> + + Add additional space after periods in NEWS. + * NEWS (2.5): here. + 2009-09-29 Joel E. Denny <jdenny@...> Use the correct conversion specifier for size_t. @@ -22,16 +27,17 @@ 2009-09-19 Alex Rozenman <rozenman@...> Keep sub-messages aligned. Fix strings for translation. - * src/location.h: (location_print): Add return value. - * src/location.c: (location_print): Return number of printed + * src/location.h (location_print): Add return value. + * src/location.c (location_print): Return number of printed characters. - * src/complain.h: Two new functions (complain_at_indent, - warn_at_indent). - * src/complain.cpp: Implement the alignment mechanism. Add new - static variable (indent_ptr). Use and update it (error_message, - complain_at_indent, warn_at_indent). - * src/scan-code.l: Fix strings for translations. Use new _indent - functions (parse_ref, show_sub_messages). + src/complain.h (complain_at_indent, warn_at_indent): Prototype + new functions. + * src/complain.cpp (indent_ptr): New static variable. + (error_message, complain_at_indent, warn_at_indent): Implement + the alignment mechanism. + * src/scan-code.l (parse_ref, show_sub_messages): Fix strings + for translations. Use new alignment mechanism. + * tests/named-ref.at: Adjust test-cases. * NEWS (2.5): Add an announcement about named references. 2009-09-17 Akim Demaille <demaille@...> diff --git a/NEWS b/NEWS index c80c40f..d585139 100644 --- a/NEWS +++ b/NEWS @@ -71,11 +71,11 @@ Bison News stmt[res] : 'if' expr[cond] 'then' stmt[then] 'else' stmt[else] ';' { $res = mk_if_stmt($cond, $then, $else); } - Location information is also accessible using @name syntax. When + Location information is also accessible using @name syntax. When accessing symbol names containing dots or dashes, explicit bracketing ($[sym.1]) must be used. - These features are experimental in this version. More user feedback + These features are experimental in this version. More user feedback will help to stabilize them. ** IELR(1) and Canonical LR(1) Support
	Re: [PATCH] Keep sub-messages aligned. Fix strings for translation. by Alex Rozenman :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Hi Joel, >> Rather than forcing the caller to deal with the indentation variable, it >> seems like it would be cleaner if warn_at automatically set the >> indentation level in a static global variable. Any subsequent >> warn_at_indent invocation would automatically use that indentation level. >> And the next warn_at invocation would reset it, etc. > > It appears that named reference message blocks have the nice property that > the location printed on the first line is not usually much shorter than > the locations printed on subsequent lines. When that's not the case, > neither your implementation nor my proposed one would indent properly. Yes, it is correct. I've also elaborated this "nice property" in my mind. I just would like to describe my motivation for the current implementation. It is 100% correct that it is impossible to implement this kind of indentation without buffering. On the other hand, it looks to me a little bit overkilling to introduce buffering just for one kind of error messages. There was no such a need until now. This understanding (that I am doing a solution for a specific case) drove me to give the client code more control on the indentation variable. The real question here: is there other places in Bison, where you needed such a complex kind of error message output ? If we will decide to implement the buffering, I can do that (the obvious price - more complex code in complain.c). -- Best regards, Alex Rozenman (rozenman@...).
	Re: [PATCH] Keep sub-messages aligned. Fix strings for translation. by Joel E. Denny-4 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Sat, 3 Oct 2009, Alex Rozenman wrote: > The real question here: is there other places in Bison, where you > needed such a complex kind of error message output ? If we will decide > to implement the buffering, I can do that (the obvious price - more > complex code in complain.c). Let's put it off as I'm short on time anyway. What we have seems to work well enough for what we're currently using it for. Thanks for that. I pushed this to master. From 3c5362b825a9d01eafe257943b7faad92ea43a05 Mon Sep 17 00:00:00 2001 From: Joel E. Denny <jdenny@...> Date: Tue, 6 Oct 2009 12:47:47 -0400 Subject: [PATCH] * TODO (Complaint submessage indentation): New. --- ChangeLog \| 4 ++++ TODO \| 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 0 deletions(-) diff --git a/ChangeLog b/ChangeLog index 90f6c23..5bbab03 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2009-10-06 Joel E. Denny <jdenny@...> + + * TODO (Complaint submessage indentation): New. + 2009-10-04 Joel E. Denny <jdenny@...> Minor code cleanup. diff --git a/TODO b/TODO index c3aac31..7f4c514 100644 --- a/TODO +++ b/TODO @@ -455,6 +455,23 @@ to bison. If you're interested, I'll work on a patch. * Better graphics Equip the parser with a means to create the (visual) parse tree. +* Complaint submessage indentation. +We already have an implementation that works fairly well for named +reference messages, but it would be nice to use it consistently for all +submessages from Bison. For example, the "previous definition" +submessage or the list of correct values for a %define variable might +look better with indentation. + +However, the current implementation makes the assumption that the +location printed on the first line is not usually much shorter than the +locations printed on the submessage lines that follow. That assumption +may not hold true as often for some kinds of submessages especially if +we ever support multiple grammar files. + +Here's a proposal for how a new implementation might look: + + http://lists.gnu.org/archive/html/bison-patches/2009-09/msg00086.html + ----- Copyright (C) 2001, 2002, 2003, 2004, 2006, 2008-2009 Free Software -- 1.5.4.3