Comments

See http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02058.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02060.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02063.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02065.html
for the previous pieces of the patch.
The fifth piece just makes the option a discriminated union. This is
absolutely required to write & re-read a persistent state (otherwise,
there is no *simple* way to know which member of the union to use).
The patch also renamed as styline & ptyline the location sub-fields of
struct type. This is for improved readability & was and is used to help
track bugs (like e.g.
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01992.html etc)
And most of the patch is just moving many definitions from gengtype.c to
gengtype.h. They need to be public for the future gengtype-state.c file.
As usual, I am attaching a relative patch to the previous (4th, 3rd,
2nd, 1st) patches and a gcc/ChangeLog entry. And for completeness the
cumulated total patch against r163612 of trunk.
The generated files did not change.
Ok for trunk?
Cheers.

See http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02058.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02060.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02063.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02065.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02068.html
for the previous pieces of the patch.
In my understanding, the files generated by gengtype patched as above do
not change from current trunk's behavior. Even better, the same files
are generated with the current trunk (r.163612-), with the current trunk
plus only the first patch piece applied, with the current trunk plus the
two first patch pieces applied, etc.
Ian Taylor, Diego Novillo, Paolo Bonzini and others, is that slicing in
several pieces enough for you? I hope yes!
We (Jeremie Salvucci & me Basile) strongly believe that the just
referenced patch pieces can be reviewed & should go into the trunk
before end of stage 1 of 4.6 i.e. current trunk.

A lot of definitions in this patch are both changed slightly and moved
to gengtype.h from gengtype.c. So with respect to this particular
patch, a proper patch splitting would be separate patches for changing
things and moving things around. Then reviewers are able to see more
easily what exactly has changed in the definitions. But I guess this
patch is small enough so you don't have to resubmit it that way.
> (string_type, scalar_nonchar, scalar_char): Definitions made> public & updated.
C has no notion of "public", what you are doing here is removing "static".
+enum typekind {
+ TYPE__NONE=0, /* Never used, so any zero-ed type is
+ invalid. */
Why not just TYPE_NONE ?
+ INFO__NONE=0, /* Never used. */
Same.
+struct options
+{
+ struct options *next;
+ const char *name;
+ enum info_kind kind;
+ /* the union below is discriminated by the 'kind' field above. */
Watch indentation.
+/* Our type structure describes all types handled by gengtype. */
+struct type
+{
...
+ struct fileloc styline;
...
+ struct fileloc ptyline;
...
+};
I think these two renames are redudant. The original "struct fileloc
line" is non-ambiguous, not to mention that using it would reduce the
patch size.
+create_type_option (options_p next, const char* name, type_p info)
+options_p
+create_nested_option (options_p next, const char* name,
+ struct nested_ptr_data* info)
These two need their own comments.
I think this patch is a great improvement to gengtype internals and I
would like very much to see it committed. I cannot approve it, but it
looks good to me with the comments addressed.
2010/8/29 Basile Starynkevitch <basile@starynkevitch.net>:
>> See http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02058.html &> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02060.html &> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02063.html &> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02065.html> for the previous pieces of the patch.>>> The fifth piece just makes the option a discriminated union. This is> absolutely required to write & re-read a persistent state (otherwise,> there is no *simple* way to know which member of the union to use).>> The patch also renamed as styline & ptyline the location sub-fields of> struct type. This is for improved readability & was and is used to help> track bugs (like e.g.> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01992.html etc)>> And most of the patch is just moving many definitions from gengtype.c to> gengtype.h. They need to be public for the future gengtype-state.c file.>> As usual, I am attaching a relative patch to the previous (4th, 3rd,> 2nd, 1st) patches and a gcc/ChangeLog entry. And for completeness the> cumulated total patch against r163612 of trunk.>> The generated files did not change.>> Ok for trunk?>>> Cheers.>> --> 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} ***>

2010/8/29 Basile Starynkevitch <basile@starynkevitch.net>:
> Ian Taylor, Diego Novillo, Paolo Bonzini and others, is that slicing in> several pieces enough for you? I hope yes!
FWIW, I think Basile's and Jeremie's submission is split well enough
for reviews.
> I am not able to break the gengtype-state.c into independent & testable> pieces. Is it acceptable to send an entire new file in a patch?
Yes.
> What should be the ChangeLog entry for an entire new file. Is just a> simple line> * gengtype-state.c: Added file.> enough
I believe so.