Re: gengtype plugin improvement last4round -patch 6 [wstate]

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

Hello -

2010/11/25 Basile Starynkevitch <basile@starynkevitch.net>:
> However I cannot make the variadic fatal_reading_state_printf a
> function, because I see no va_arg argument accepting function in
> errors.h,

Bummer. Well fatal_reading_state_printf is going to be a macro then.

>> + Â/* We write the state here. ÂIt could eventually happen that the
>> + Â Â state file is written after some plugin files have been parsed,
>> + Â Â perhaps to enlarge the state file for other plugins needs. ÂBut
>> + Â Â this is an uncommon scenario. Â*/
>>
>> Probably not the best place to discuss what may or may not happen in
>> the future. More like wiki material.
>
> I respectfully disagree with Laurynas comments.  We have to explain why
> write_state is called precisely at this place in main (intuitively, Jeremie
> & me placed it elsewhere, and that was wrong). So I shortened the comment,
> but did say something.
>
> The uncommon scenario (this word does not appear any more in the patch) is:
> A plugin foo.so is so common and widely used that it adds new GTY-ed stuff
> and redistribute a gtype-foo.state file augmented with the GTY-ed types
> defined by that plugin. Other plugins foobar.so and foodee.so are based
> upon foo.so (that is, they require the foo.so plugin to be loaded before
> them) and use gtype-foo.state as the state file. Laurynas, could you accept
> the small comment that remains?  I strongly think we have to say something!

I am very sorry. Originally I understood this comment "It could
eventually happen" to mean something like a TODO item, i.e. possible
future gengtype improvement. I apologize for the misunderstanding.

However I am looking at main() and have difficulty understanding: what
was, as you say, the more intuitive place for write_state call that
was wrong? For me the current one looks like the most intuitive: after
all reading is done and before writing any output. Second, can you
explain bit more this scenario of plugin depending on another plugin?
If I understand you correctly, what happens is that for the second
plugin we first read the state and augment it by parsing the second
plugin files, then write the augmented state. If that's correct, then
perhaps replacing the comment here:

  if (plugin_output_filename)
    {
      ...
      /* Parse our plugin files */
      for (ix = 0; ix < nb_plugin_files; ix++)
        parse_file (get_input_file_name (plugin_files[ix]));

with /* Parse our plugin files and augment the state.  */

and then the comment that started all this discussion can become

/* The state at this point is read from the state input file or by
parsing source files and optionally augmented by parsing plugin source
files.  Write it now.  */

Do you find this suggestion good enough?

> PS. Before a documentation patch, we still need a minor Makefile.in patch
> to install gengtype appropriately. Alexander Oliva suggested me at the
> GCC summit to install gengtype, which becomes a user visible utility,
> in $(DESTDIR)$(libexecsubdir)/ where cc1 is already sitting.

Other comments:

+struct state_token_st
+{
...
+  union		/* discriminated by stok_kind! */

Single unaligned comment.

Please change write_state_type switch default label to gcc_unreachable call.

write_state_typedefs header comment: "writting" -> "writing"

In most of the reading routines I'd suggest merging declarations and
initializations:

+  struct state_token_st *t0;
+  struct state_token_st *t1;
+  struct state_token_st *t2;
+
+  t0 = peek_state_token (0);
+  t1 = peek_state_token (1);
+  t2 = peek_state_token (2);

struct state_token_st *t0 = peek_state_token(0);
etc.

The type_count handling in gengtype.c suggests factoring out some new
function new_type or similar that only does

type_count++;
struct type *result = XCNEW (struct type);
si->state_number = -type_count;
return result;

It is strictly optional: if you wish, you can do a follow-up patch for
this change. It is not necessary to make this change in the current
patch.

The type_lineloc in gengtype.h is only used in gengtype-state, so
please make it a static function there.

I suspect that #includes in gengyte-state.c were copy-pasted from
gengtype.c and that it is unnecessary to include getopt.h nor
xregex.h.

The gengtype parts are OK with these changes (the comment issue in
main() can be resolved any way you like and can be adjusted in a
follow-up if needed), The Makefile parts are obviously OK unless build
maintainers say otherwise.

Thanks,
-- 
Laurynas