This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

<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.]

> 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.

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.

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

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.

 - 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);
> +   abort ();
> + #endif

   instead do something similar to error_recursion ().  In particular,
   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.

 - 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).

 - 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.

 - 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.

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]