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

[Joel E. Denny]

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  <address@hidden>
> +
> +     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  <address@hidden>
  
        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.