Re: Support for %d$c format specifier in diagnostics.c

[Chiaki <ishikawa at yk dot rim dot or dot jp>]

Zack Weinberg wrote:
> <ishikawa@duron.erephon> writes:
>
> [Incidentally, could you please fix your Reply-To: and/or From: line
> so that we don't have to edit the message headers when we reply to
> you? Thanks in advance.]
Sorry about the mail setup problem.
I will try to see what I can do.

> > Here is the latest patch to support positional format specifiers
> > in GCC's diagnostics output.
>
>
> Okay, so first, a procedural issue:  This patch is a bug fix.  We have
> .po files which try to use this construct, and since it's unimplemented, 
> the compiler ICEs.  Further, you submitted it well in advance of the
> deadline for non-bugfix changes, and the major reason why it isn't in
> yet is that you needed to take care of your copyright paperwork.  And
> while it's quite invasive, it only touches the diagnostic printer.  So 
> I say that the patch can be accepted for 3.4.
I will try to accommodate what can be done
to meet your revision request and code style concerns.
(There were things I didn't kwow like the capital letter version of
fucnitions in ctype.h. Are there GCC-specific coding style documents
somewhere?).

> However, I see a number of technical problems with the code as it
> stands, and I'm afraid I'm going to have to ask for a major revision
> before it is committed.
>
> I have two fundamental problems with the code.  First, the algorithm
> for handling positional specifiers is too complicated.  Second, you
> should not preserve the old algorithm alongside the new one - I
> understand that you were trying to avoid bugs, but by introducing two
> code paths one of which is rarely executed, you actually increase the
> probability that bugs will occur and go unnoticed.

Now that you took the time to figure out what the code does,
and get the concern, which I share by the way, that the
somewhat duplicated code path (unavoidable in my first approach)
I guess that I should remove the duplication by following the suggested
approach.

> The algorithm for handling positional specifiers is too complicated
> because you are trying to implement a more general semantic for them
> than we need.  We do not need the ability to refer to a given argument
> more than once with different format characters.  This being the case,
> a much simpler algorithm can be used:
>
> 1) Scan the string, count the number of format specifiers, allocate an
>    array of char * with that many indexes, scan the string again and
>    fill in the array with format specifiers.  So for the string
>
>    "%1$s no tiene soporte para %2$s con el formato %4$s `%%%3$c'"
>
>    the array constructed would be
>
>    { "s", "s", "c", "s" }
>
>    (the specifier strings can be just pointers into the original string).
>    If you encounter two different specifiers with the same index,
>    abort.  Note that %% doesn't get an entry in this array.
>
>    Note that you would do the same thing for the untranslated version,
>    which doesn't use positional format specifiers:
>
>    "%s does not support %s with the `%%%c' %s format"
>
> 2) Walk down the array calling the rendering function for each.  This
>    function should take a pointer to the format specifier, be given
>    access to the diagnostic context, and return a pointer to the
>    rendered string (the diagnostic context contains an obstack where
>    space for this string should be allocated).  The pointer to the
>    rendered string replaces the pointer to the format specifier in the
>    array.
>
> 3) Scan the string, writing out literal text; when you encounter a
>    format specifier, just copy out the string from the array.
>
> This should be easy to implement, and in addition, it should require
> far fewer changes to the front-end-specific diagnostic formatters.
>
> Notice also that this allows the special handling for %H and %J to go
> away.  The rendering function is called for every argument index
> before any text is ever output, so it can just tweak the location
> information in the diagnostic_context as necessary.  We can even make
> the C++ front end's old %+D notation work again - cleanly - if we want.
>
> Please reimplement your patch along these lines and resubmit it.  
> I should warn you that I saw other problems with your code as well.
> I'm not going to go into detail about these since I'm asking you to
> reimplement, but:
>
>  - Make sure all lines are wordwrapped to 80 columns.

(I am not sure whether this referes to the original coding style or
 maybe the mail handler wrapping the code at unwanted positions?)


>  - Comments should discuss what the code does and why, but they
>    should not discuss the history of the code or your thought process
>    during development.
>
>  - Do not do this: 
>
>
> > +   fprintf (stderr, "Problem with output formatting: %s\n", a);
> > +   fprintf (stderr, "format=<<%s>>\n", text->original_format_spec);
> > + #ifdef ENABLE_CHECKING
> > +   abort ();
> > + #endif
>
>
>    instead do something similar to error_recursion ().  

I did this, but I guess I can obtain nicer output
for contacting GCC support mailing list, etc. by calling
error_recusion()(?).


>    use fnotice instead of fprintf, exit(FATAL_EXIT_CODE) instead of
>    abort, don't guard the exit with ENABLE_CHECKING, and make sure to
>    print out the bug_report_request text.

OK. Again, a concise GCC coding style  manual/document will be
highly useful.

>  - Do not use <ctype.h> anywhere in GCC.  Use the capitalized versions
>    of these functions which are defined in include/safe-ctype.h (this
>    header is included by system.h).

Ditto.


>  - Please do remember that processing of positional format specifiers
>    and processing of normal format specifiers should follow the _same_
>    code paths to the maximum extent possible.

Agreed.

>  - Create a new directory gcc/testsuite/gcc.unit-tests and put your
>    test code in there as a separate file.  Don't worry about there
>    being no harness for running that test.  We'll eventually come up
>    with one, and having a test there for the harness to use is
>    encouragement to do so.

OK.

> I want to say that I appreciate your working on this patch and I do
> intend it to go into 3.4 after it's revised to everyone's satisfaction.

Thank you for your encouraging words.
This feature (positional parameter support) has been long due.
So I will see what I can do.

I have been away from my PC for more than a week due to a
business trip,  but
when is exactly the 3.4 release time frame?

>


--
int main(void){int j=2003;/*(c)2003 cishikawa. */
char t[] ="<CI> @abcdefghijklmnopqrstuvwxyz.,\n\"";
char *i ="g>qtCIuqivb,gCwe\np@.ietCIuqi\"tqkvv is>dnamz";
while(*i)((j+=strchr(t,*i++)-(int)t),(j%=sizeof t-1),
(putchar(t[j])));return 0;}/* under GPL */