Patchwork gengtype: fix output_code_enum.

login
register
mail settings
Submitter Basile Starynkevitch
Date Aug. 27, 2010, 6:45 p.m.
Message ID <1282934717.625.33.camel@glinka>
Download mbox | patch
Permalink /patch/62865/
State New
Headers show

Comments

Basile Starynkevitch - Aug. 27, 2010, 6:45 p.m.
Hello All,

A half line patch, discussed on
http://gcc.gnu.org/ml/gcc/2010-08/msg00396.html
http://gcc.gnu.org/ml/gcc/2010-08/msg00386.html 

Attached files:
  the diff
  the gcc/ChangeLog entry.

Ok for trunk?

Cheers. [Basile Starynkevitch & Jeremie Salvucci]
Laurynas Biveinis - Aug. 27, 2010, 6:55 p.m.
I believe the patch is OK, but:
- do not include mailing list links in the ChangeLog
- Capitalize the "Test"

Thank you for catching this and Ian for the explanation.

2010/8/27 Basile Starynkevitch <basile@starynkevitch.net>:
> Hello All,
>
> A half line patch, discussed on
> http://gcc.gnu.org/ml/gcc/2010-08/msg00396.html
> http://gcc.gnu.org/ml/gcc/2010-08/msg00386.html
>
> Attached files:
>  the diff
>  the gcc/ChangeLog entry.
>
> Ok for trunk?
>
> Cheers. [Basile Starynkevitch & Jeremie Salvucci]
>
> --
> Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
> email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
> 8, rue de la Faiencerie, 92340 Bourg La Reine, France
> *** opinions {are only mine, sont seulement les miennes} ***
>
Ian Taylor - Aug. 27, 2010, 7:58 p.m.
Basile Starynkevitch <basile@starynkevitch.net> writes:

> Hello All,
>
> A half line patch, discussed on
> http://gcc.gnu.org/ml/gcc/2010-08/msg00396.html
> http://gcc.gnu.org/ml/gcc/2010-08/msg00386.html 
>
> Attached files:
>   the diff
>   the gcc/ChangeLog entry.
>
> Ok for trunk?
>
> Cheers. [Basile Starynkevitch & Jeremie Salvucci]


2010-08-27  Basile Starynkevitch  <basile@starynkevitch.net>
	    Jeremie Salvucci  <jeremie.salvucci@free.fr>
	http://gcc.gnu.org/ml/gcc/2010-08/msg00396.html

	* gengtype.c (output_type_enum): test the right union member.

Please omit the HTTP link, and capitalize "test".

This is OK with those ChangeLog changes.

Thanks.

Ian
Basile Starynkevitch - Aug. 27, 2010, 8:34 p.m.
> 
> Please omit the HTTP link, and capitalize "test".
> 
> This is OK with those ChangeLog changes.


I did comply, and committed 163596.

However, I am curious.  Why is it wrong to make a link to something
under gcc.gnu.org.  For bugs with a PR, there is a number which is
logically such a link.

Cheers.
Ian Taylor - Aug. 27, 2010, 9:49 p.m.
Basile Starynkevitch <basile@starynkevitch.net> writes:

> However, I am curious.  Why is it wrong to make a link to something
> under gcc.gnu.org.  For bugs with a PR, there is a number which is
> logically such a link.

The ChangeLog says what has changed, it does not say why it has changed.
If it is necessary to say why something has changed, the place for that
is in a comment in the code.

You are right that the bug numbers in the ChangeLog entries are an
exception.  They are there to cause the patch to be automatically added
to the bug report.

You may, if you like, present an argument for why we should have the
ChangeLog file link to the mailing list.  But then we should do it for
pretty much every change, not just yours.  Unless and until we make that
general change, you should follow the existing conventions.

Of course, these days, with use of source code control and fast search
of code repositories, it's not clear that we need a the current format
of the ChangeLog file at all.

Ian

Patch

Index: gcc/gengtype.c
===================================================================
--- gcc/gengtype.c	(revision 163593)
+++ gcc/gengtype.c	(working copy)
@@ -2531,7 +2531,7 @@  write_types_process_field (type_p f, const struct
 static void
 output_type_enum (outf_p of, type_p s)
 {
-  if (s->kind == TYPE_PARAM_STRUCT && s->u.s.line.file != NULL)
+  if (s->kind == TYPE_PARAM_STRUCT && s->u.param_struct.line.file != NULL)
     {
       oprintf (of, ", gt_e_");
       output_mangled_typename (of, s);