Re: gengtype improvements for plugins. patch 5/N [typedopt]

[Laurynas Biveinis <laurynas dot biveinis at gmail dot com>]

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} ***
>



-- 
Laurynas