gengtype improvements for plugins, completed! patch 1/N [declprog]

Hello All,

[join work by Basile Starynkevitch & Jeremie Salvucci]

References: http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02058.html 
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02050.html
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02051.html
http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00616.html

The first piece [declprog] of our series of patches moves many private
definitions, types, functions from gengtype.c to gengtype.h & provide
a GNU friendly gengtype program invocation. Since a later patch of the
same serie implement the state persistency in a separate file
gengtype-state.c (see our future patch 6/N[wstate]) a lot of variables
& types which used to be internal to gengtype.c are now publicly
available in gengtype.h.


We did incorporate Paolo Bonzini's & Ralf Wildenhues' suggestions on
build/version.o in Makefile.in from
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02114.html &
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02050.html

We did improve the help message as suggested by Laurynas Biveinis in
http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00207.html


Please notice an important point in this patch (and other patches of
this serie): we program defensively, since we feel that gengtype is
very difficult to understand.  In particular, we added several
debugging tricks, notably the DBGPRINTF & DBGPRINT_COUNT_TYPE macros,
whose sole side-effect is to produce debugging output when gengtype is
given the --debug (or -D) program argument.  This feature (only
available when ENABLE_CHECKING is configured) is deemed necessary
because gengtype code is really difficult to grasp and to debug.  It
is complementary to the dump_everything & friends functions already
incorporated into gengtype and contributed by Laurynas Biveinis:
Laurynas dump feature is useful not only to debug and enhance gengtype
itself (a not trivial task, believe us!) but may also be useful by
gengtype users (e.g. plugins developers).  In contrast, our DBGPRINTF
... macros are mostly useful to debug & understand the processing of
gengtype itself.  Of course, we could remove them, but we strongly
feel that they could be useful to some other gengtype hacker (the same
can be said of Laurynas dump facility).  So to the attention of the
allmighty reviewers: please measure the pro & conses before asking us
to remove these DBGPRINTF macros.  We really hope they will stay... 

################################################################
gcc/ChangeLog entry:

2010-09-08  Jeremie Salvucci  <jeremie.salvucci@free.fr>
	    Basile Starynkevitch  <basile@starynkevitch.net>

	* gengtype.c:  Include getopt.h and version.h.

	(lang_bitmap, struct outf, outf_p)
	(get_output_file_with_visibility, oprintf): Definitions moved to
	gengtype.h
	(output_files, header_file, srcdir, srcdir_len, this_file, do_dump): No
	more static variables.
	(do_debug): New.
	(dbgprint_count_type_at): Added new function.
	(gengtype_long_options): New.
	(print_usage, print_version, parse_program_options): New.
	(main): Call parse_program_options, and removed old option
	handling code.  Added some debug output.

	* gengtype.h:  Updated copyright year.
	(lang_bitmap, struct outf, outf_p, header_file, oprintf)
	(get_output_file_with_visibility, srcdir, srcdir_len, do_dump):
	Moved from gengtype.c to here.
	(do_debug, read_state_filename, write_state_filename): New variables.
	(DBGPRINTF, DBGPRINT_COUNT_TYPE): New macros.

	* Makefile.in:
	(REVISION): Always defined.
	(version.o): Removed ifdef REVISION_c.
	(s-gtype): Pass arguments to build/gengtype program.
	(build/version.o): Added building rule.
	(build/gengtype$(build_exeext)): Added build/version.o.
################################################################

Attached file: gengtype_patch_1_of_N__declprog-trunkrev-163987.diff
the diff file against trunk rev 163987.

Ok for trunk?

Regards.

+/* Our ource directory and its length.  */

Source

@@ -249,7 +246,7 @@ static lang_bitmap
 get_lang_bitmap (const char *gtfile)
 {

-  if (gtfile == this_file)
+  if (gtfile == this_file || gtfile == system_h_file)
     /* Things defined in this file are universal.  */
     return (((lang_bitmap)1) << num_lang_dirs) - 1;
   else

Can you explain this change? It looks sensible, but looks sort of
unrelated to the rest of the patch. Was it a real bug before?

Also, please add a comment, probably not here but next to some gtfile
definition, why comparing const char * for pointer equality instead of
using strcmp is OK.

+	/* In plugin mode we require some input files. */
+	int i = 0;
+	if (optind >= argc)
+	    fatal("no source files given in plugin mode");
+	nb_plugin_files = argc - optind;
+	for  (i = 0; i < (int) nb_plugin_files; i++)

Just like the last time, I still don't get it. The usage help says:

+    printf ("\t -P | --plugin <output-file>  \t#  Generate for a plugin.\n");

And this code suggests
-P <output-file> <input-file> <input-file> ...

I think the usage help must mention the input files.

   /* These types are set up with #define or else outside of where
-     we can see them.  */
+	 we can see them.  We should initialize them before calling

Suspicious formatting.

+/* Structure representing an output file.  */
+struct outf
+{
+    struct outf *next;
+    const char *name;
+    size_t buflength;
+    size_t bufused;
+    char *buf;
+};
+typedef struct outf* outf_p;

Formatting. Also I am not terribly happy about hiding a pointer in a
typedef, but it's not prohibited I guess.

+/* Variable used for reading and writing the state. */
+extern char *read_state_filename;
+extern char *write_state_filename;

Make these const (just like srcdir)

2010/9/8 Basile Starynkevitch <basile@starynkevitch.net>:
>
> Hello All,
>
> [join work by Basile Starynkevitch & Jeremie Salvucci]
>
> References: http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02058.html
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02050.html
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02051.html
> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00616.html
>
> The first piece [declprog] of our series of patches moves many private
> definitions, types, functions from gengtype.c to gengtype.h & provide
> a GNU friendly gengtype program invocation. Since a later patch of the
> same serie implement the state persistency in a separate file
> gengtype-state.c (see our future patch 6/N[wstate]) a lot of variables
> & types which used to be internal to gengtype.c are now publicly
> available in gengtype.h.
>
>
> We did incorporate Paolo Bonzini's & Ralf Wildenhues' suggestions on
> build/version.o in Makefile.in from
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02114.html &
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02050.html
>
> We did improve the help message as suggested by Laurynas Biveinis in
> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00207.html
>
>
> Please notice an important point in this patch (and other patches of
> this serie): we program defensively, since we feel that gengtype is
> very difficult to understand.  In particular, we added several
> debugging tricks, notably the DBGPRINTF & DBGPRINT_COUNT_TYPE macros,
> whose sole side-effect is to produce debugging output when gengtype is
> given the --debug (or -D) program argument.  This feature (only
> available when ENABLE_CHECKING is configured) is deemed necessary
> because gengtype code is really difficult to grasp and to debug.  It
> is complementary to the dump_everything & friends functions already
> incorporated into gengtype and contributed by Laurynas Biveinis:
> Laurynas dump feature is useful not only to debug and enhance gengtype
> itself (a not trivial task, believe us!) but may also be useful by
> gengtype users (e.g. plugins developers).  In contrast, our DBGPRINTF
> ... macros are mostly useful to debug & understand the processing of
> gengtype itself.  Of course, we could remove them, but we strongly
> feel that they could be useful to some other gengtype hacker (the same
> can be said of Laurynas dump facility).  So to the attention of the
> allmighty reviewers: please measure the pro & conses before asking us
> to remove these DBGPRINTF macros.  We really hope they will stay...
>
> ################################################################
> gcc/ChangeLog entry:
>
> 2010-09-08  Jeremie Salvucci  <jeremie.salvucci@free.fr>
>            Basile Starynkevitch  <basile@starynkevitch.net>
>
>        * gengtype.c:  Include getopt.h and version.h.
>
>        (lang_bitmap, struct outf, outf_p)
>        (get_output_file_with_visibility, oprintf): Definitions moved to
>        gengtype.h
>        (output_files, header_file, srcdir, srcdir_len, this_file, do_dump): No
>        more static variables.
>        (do_debug): New.
>        (dbgprint_count_type_at): Added new function.
>        (gengtype_long_options): New.
>        (print_usage, print_version, parse_program_options): New.
>        (main): Call parse_program_options, and removed old option
>        handling code.  Added some debug output.
>
>        * gengtype.h:  Updated copyright year.
>        (lang_bitmap, struct outf, outf_p, header_file, oprintf)
>        (get_output_file_with_visibility, srcdir, srcdir_len, do_dump):
>        Moved from gengtype.c to here.
>        (do_debug, read_state_filename, write_state_filename): New variables.
>        (DBGPRINTF, DBGPRINT_COUNT_TYPE): New macros.
>
>        * Makefile.in:
>        (REVISION): Always defined.
>        (version.o): Removed ifdef REVISION_c.
>        (s-gtype): Pass arguments to build/gengtype program.
>        (build/version.o): Added building rule.
>        (build/gengtype$(build_exeext)): Added build/version.o.
> ################################################################
>
> Attached file: gengtype_patch_1_of_N__declprog-trunkrev-163987.diff
> the diff file against trunk rev 163987.
>
> Ok for trunk?
>
> Regards.
> --
> 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 mines, sont seulement les miennes} ***
>

On Thu, 9 Sep 2010 05:52:24 +0300
Laurynas Biveinis <laurynas.biveinis@gmail.com> wrote:

> +/* Our ource directory and its length.  */
> 
> Source
> 
> @@ -249,7 +246,7 @@ static lang_bitmap
>  get_lang_bitmap (const char *gtfile)
>  {
> 
> -  if (gtfile == this_file)
> +  if (gtfile == this_file || gtfile == system_h_file)
>      /* Things defined in this file are universal.  */
>      return (((lang_bitmap)1) << num_lang_dirs) - 1;
>    else
> 
> Can you explain this change? It looks sensible, but looks sort of
> unrelated to the rest of the patch. Was it a real bug before?

We did introduce the system_h_file. It could happen that
get_lang_bitmap is never called on it, but if it where called, it could
perhaps SEGV without the gtfile == system_h_file test, since
system_h_file is like this_file, a const. 

This brings a question to the reviewer: I have the intuitive feeling
that the gtfile == system_h_file is missing, but I cannot prove that.
And the next patch will replace all the messy input file code with
something less ugly, so this change is a very temporary kludge.  Should
I take days to try to understand why the current gengtype is working (I
believe it is mostly chance).  I'm not sure to have time for that,
given that the next patch which is essentially
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02063.html rephrased a
little bit, will remove that mess.


And as you probably know, this patch piece is a very temporary fix. The
next patch [not yet sent in the completed form, but essentially same as 
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02063.html rephrased a
little bit) is adding a real input_file structure, which removes all
those nasty byte & bits manipulation, and the insane hypothesis that
every file path have four bytes before it for its lang_bitmap.  My
feeling is that this hypothesis is already violated by "system.h" and
that gengtype happens to work just by chance (it did already happen for
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01992.html which we did
commit later after typo corrections). But I don't want to spend days to
investigate that.  The next patch is clearing this mess thru a real
input_file structure.

> 
> Also, please add a comment, probably not here but next to some gtfile
> definition, why comparing const char * for pointer equality instead of
> using strcmp is OK.

I believe there is such a comment somewhere in the old trunk gengtype.
But the input files as strings are disappearing in the next patch. 
> 
> +	/* In plugin mode we require some input files. */
> +	int i = 0;
> +	if (optind >= argc)
> +	    fatal("no source files given in plugin mode");
> +	nb_plugin_files = argc - optind;
> +	for  (i = 0; i < (int) nb_plugin_files; i++)
> 
> Just like the last time, I still don't get it. The usage help says:
> 
> +    printf ("\t -P | --plugin <output-file>  \t#  Generate for a plugin.\n");
> 

Oh, I understand what you mean.  Our usage don't mention input files
for plugins! Will fix that.

> And this code suggests
> -P <output-file> <input-file> <input-file> ...
> 
> I think the usage help must mention the input files.
> 
>    /* These types are set up with #define or else outside of where
> -     we can see them.  */
> +	 we can see them.  We should initialize them before calling
> 
> Suspicious formatting.
> 
> +/* Structure representing an output file.  */
> +struct outf
> +{
> +    struct outf *next;
> +    const char *name;
> +    size_t buflength;
> +    size_t bufused;
> +    char *buf;
> +};
> +typedef struct outf* outf_p;
> 
> Formatting. Also I am not terribly happy about hiding a pointer in a
> typedef, but it's not prohibited I guess.

I am not happy neither, but the current trunk's gengtype already had
the "typedef struct outf* outf_p;" definition. I agree with you, it is
disgusting.

If I was not scared by reviewers availability, I would on the contrary
use "typedef struct output_file_st output_file;"
and use "output_file*" everywhere instead of "outf_p". 
However, doing that would increase significantly the length of the
patch, and I am very scared of discouraging potential reviewers. If
some reviewer (i.e. a person invested with the power to OK that patch)
would say to me that it is welcome, I would be very happy to make the
change.  But I am scared of making patches bigger than what a reviewer
can digest, and I do confess that reading gengtype code is not fun at
all.  Even with our patches, gengtype remains ugly.


I think it is better if I send the next patches as soon as possible. I
am not forgetting or ignoring your feedback, but I believe that to keep
reviewer's attention I should focus on sending this complete round of
patch in totality first.

After having sent the complete serie of "completed!" patches, I will
remake a third round.

I still don't understand who has a reviewer hat, and I still don't
understand if our patches have any chance to go into trunk before end
of stage 1 which is scheduled for october 27th.

And while I do understand the need to commit code of high quality, for
gengtype specifically I believe the code was so badly written that
reviewers should be a little less harsh with us. We do know that our
code is not perfect, but gengtype is so awefully coded that our patch
serie is still a minor improvement.

To say it frankly, the code existing in trunk's gengtype is so aweful 
that it should never had been accepted in the first place. Apparently,
at that time (2002? perhaps) the philosophy was: if it happens to work,
commit it.  But today the requirements are much bigger! 
However, I have not enough time to understand why the current trunk
gengtype is working. In my view, it is by chance (there are probably
several bugs like
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01992.html inside it).
And I certainly don't have time to correct every such bug inside.




Regards.

2010/9/9 Basile Starynkevitch <basile@starynkevitch.net>:
> This brings a question to the reviewer: I have the intuitive feeling
> that the gtfile == system_h_file is missing, but I cannot prove that.
> And the next patch will replace all the messy input file code with
> something less ugly, so this change is a very temporary kludge.

In that case might as well just drop this change, since your patches
will probably go into the trunk all at once.

>> Also, please add a comment, probably not here but next to some gtfile
>> definition, why comparing const char * for pointer equality instead of
>> using strcmp is OK.
>
> I believe there is such a comment somewhere in the old trunk gengtype.
> But the input files as strings are disappearing in the next patch.

True. Please disregard my comment then.

>> +typedef struct outf* outf_p;
>>
>> Formatting. Also I am not terribly happy about hiding a pointer in a
>> typedef, but it's not prohibited I guess.
>
> I am not happy neither, but the current trunk's gengtype already had
> the "typedef struct outf* outf_p;" definition. I agree with you, it is
> disgusting.
>
> If I was not scared by reviewers availability, I would on the contrary
> use "typedef struct output_file_st output_file;"
> and use "output_file*" everywhere instead of "outf_p".
> However, doing that would increase significantly the length of the
> patch, and I am very scared of discouraging potential reviewers. If
> some reviewer (i.e. a person invested with the power to OK that patch)
> would say to me that it is welcome, I would be very happy to make the
> change.  But I am scared of making patches bigger than what a reviewer
> can digest, and I do confess that reading gengtype code is not fun at
> all.  Even with our patches, gengtype remains ugly.

Follow-up patches are welcome, and they don't scare reviewers away ;)

> And while I do understand the need to commit code of high quality, for
> gengtype specifically I believe the code was so badly written that
> reviewers should be a little less harsh with us.

I don't understand. Is anyone harsh with you? For the record, I am not
asking to rewrite gengtype to improve its design. If my comments
happen to touch things that are simply unchanged by your patches, then
they are strictly optional (and perhaps best done as followup
patches). And I don't think that asking for proper code formatting is
harsh in any way ;)

> We do know that our
> code is not perfect, but gengtype is so awefully coded that our patch
> serie is still a minor improvement.

I completely agree.

> To say it frankly, the code existing in trunk's gengtype is so aweful
> that it should never had been accepted in the first place. Apparently,
> at that time (2002? perhaps) the philosophy was: if it happens to work,
> commit it.  But today the requirements are much bigger!

I think it went in without reviews, which, as you are experiencing,
haunts us 8 years later:
http://gcc.gnu.org/ml/gcc-patches/2002-06/msg00328.html
http://gcc.gnu.org/ml/gcc-patches/2002-06/msg00331.html

On Thu, 9 Sep 2010 09:35:55 +0300
Laurynas Biveinis <laurynas.biveinis@gmail.com> wrote, citing me Basile:

[....]

> > If I was not scared by reviewers availability, I would on the contrary
> > use "typedef struct output_file_st output_file;"
> > and use "output_file*" everywhere instead of "outf_p".
> > However, doing that would increase significantly the length of the
> > patch, and I am very scared of discouraging potential reviewers. If
> > some reviewer (i.e. a person invested with the power to OK that patch)
> > would say to me that it is welcome, I would be very happy to make the
> > change.  But I am scared of making patches bigger than what a reviewer
> > can digest, and I do confess that reading gengtype code is not fun at
> > all.  Even with our patches, gengtype remains ugly.
> 
> Follow-up patches are welcome, and they don't scare reviewers away ;)

What exactly do you mean by "follow-up patches" in this particular
context? I have several meanings in mind, and I cannot chose the best
one.

> 
> > And while I do understand the need to commit code of high quality, for
> > gengtype specifically I believe the code was so badly written that
> > reviewers should be a little less harsh with us.
> 
> I don't understand. Is anyone harsh with you? For the record, I am not
> asking to rewrite gengtype to improve its design. If my comments
> happen to touch things that are simply unchanged by your patches, then
> they are strictly optional (and perhaps best done as followup
> patches). And I don't think that asking for proper code formatting is
> harsh in any way ;)

Maybe "harsh" was a wrong word. I probably just meant "strong" or
"severe" or "stern". English is not my native language, and I have to
look into dictionnaries (and I am not doing often enough that effort).

In particular, I find strong the requirement of explaining
any temporary fix which will be dismantled by the next patch. And such
fixes are implied by the requirement of making gengtype still work as
before at every piece of the patch set.  Our work is seperated in
pieces of patch for convenience of reviewers.  It would be much much
easier to us to be permitted to send a single big patch all at once,
with a long message explaining every part of it.  But apparently the
GCC community or reviewers don't want that (even if it seems to have
happened in the past).  The requirement to have gengtype working at
every patch piece is in retrospect a very strong one even if I do
understand it.  The requirement to explain every kludge needed for that
is unrealistic. It requires us to understand why gengtype is working
currently (I am not able to do that), and the only good answer today is
"by accident".


Please accept my apologies for using the wrong word "harsh".
Proper code formatting is a must, but I cannot understand how to
achieve that simply & reliably. So far, I just manually read several
times the code before sending, but my brain is too familiar with my own
patch. I would be very tempted to run "indent -gnu gengtype.c" but I
feel it will add a lot of noise in the patch. Or should I run it?

Cheers.

2010/9/9 Basile Starynkevitch <basile@starynkevitch.net>:
> On Thu, 9 Sep 2010 09:35:55 +0300
> Laurynas Biveinis <laurynas.biveinis@gmail.com> wrote, citing me Basile:
>
> [....]
>
>> > If I was not scared by reviewers availability, I would on the contrary
>> > use "typedef struct output_file_st output_file;"
>> > and use "output_file*" everywhere instead of "outf_p".
>> > However, doing that would increase significantly the length of the
>> > patch, and I am very scared of discouraging potential reviewers. If
>> > some reviewer (i.e. a person invested with the power to OK that patch)
>> > would say to me that it is welcome, I would be very happy to make the
>> > change.  But I am scared of making patches bigger than what a reviewer
>> > can digest, and I do confess that reading gengtype code is not fun at
>> > all.  Even with our patches, gengtype remains ugly.
>>
>> Follow-up patches are welcome, and they don't scare reviewers away ;)
>
> What exactly do you mean by "follow-up patches" in this particular
> context? I have several meanings in mind, and I cannot chose the best
> one.

A separate, self-contained after you get your main batch accepted,
patch which would just change
typedef struct foo * foo_p;
to
typedef struct foo foo;
and change all usages accordingly.

> In particular, I find strong the requirement of explaining
> any temporary fix which will be dismantled by the next patch. And such
> fixes are implied by the requirement of making gengtype still work as
> before at every piece of the patch set.

Oh. I might be wrong, but I don't think that's necessary. You have a
set of patches which are interdependent. I think it is fine for an
arbitrary subset of these patches to break something as long as the
end result is OK. Can you point to where you were told to make sure
that gengtype works as before _at every step_?

> Proper code formatting is a must, but I cannot understand how to
> achieve that simply & reliably.

By using a tool that's configured to correctly format what you type.
http://gcc.gnu.org/wiki/FormattingCodeForGCC

> I would be very tempted to run "indent -gnu gengtype.c" but I
> feel it will add a lot of noise in the patch. Or should I run it?

Please don't, that only makes reviewer's life harder and breaks svn blame.

On Thu, Sep 09, 2010 at 10:23:15AM +0300, Laurynas Biveinis wrote:
> 2010/9/9 Basile Starynkevitch <basile@starynkevitch.net>:
> > On Thu, 9 Sep 2010 09:35:55 +0300
> 
> > In particular, I find strong the requirement of explaining
> > any temporary fix which will be dismantled by the next patch. And such
> > fixes are implied by the requirement of making gengtype still work as
> > before at every piece of the patch set.
> 
> Oh. I might be wrong, but I don't think that's necessary. You have a
> set of patches which are interdependent. I think it is fine for an
> arbitrary subset of these patches to break something as long as the
> end result is OK. Can you point to where you were told to make sure
> that gengtype works as before _at every step_?

In http://gcc.gnu.org/ml/gcc/2010-08/msg00353.html Ian Taylor wrote in
a reply to my questions

>> An ideal sequence of patches is a series of patches which may be applied
>> in order.  At each stage the tree should be buildable.  The patches
>> don't have to be independent--they can be applied in a specific
>> order--but they should each work.

I understood the above paragraph from Ian as requirement that gengtype
should always work at every stage, i.e. _at every step_ to quote
Laurynas. 

Maybe I misunderstood, or took as a requirement what is just an ideal
wish, that is something which is not realistically achievable but just
wanted in a Platonic world which does not exist in GCC.




And believe me, gengtype, even with our patch set, is *very far* from
being ideal. It will remain crappy forever

  because garbage collection is a *global* property of a program, and
  there is still no consensus within the GCC community about it; my
  "purist" view is that the GCC community should require every
  dynamically allocated data to be garbage collected with almost no
  exceptions, and then implement the tools [GC support à la Ggc but
  better, code generators much better than gengtype] to make that
  happen.  But I do know that most people disagree with my disruptive
  & extreme position.  Notice that transition to C++ could help and be
  an incentive for better garbage collection (C++ permit virtual
  functions as marking routines and could wrap nicely local GC-ed
  pointers).  But in general, as long as there is no strong consensus
  on any *global* property of GCC, we won't be able to implement it
  cleanly and robustly.  This is true of every *global* property or
  feature of GCC, e.g. memory management, organization of passes, code
  formatting style, or GCC documentation.
 

Cheers.

2010/9/9 Basile Starynkevitch <basile@starynkevitch.net>:
>> Oh. I might be wrong, but I don't think that's necessary. You have a
>> set of patches which are interdependent. I think it is fine for an
>> arbitrary subset of these patches to break something as long as the
>> end result is OK. Can you point to where you were told to make sure
>> that gengtype works as before _at every step_?
>
> In http://gcc.gnu.org/ml/gcc/2010-08/msg00353.html Ian Taylor wrote in
> a reply to my questions
>
>>> An ideal sequence of patches is a series of patches which may be applied
>>> in order.  At each stage the tree should be buildable.  The patches
>>> don't have to be independent--they can be applied in a specific
>>> order--but they should each work.

I think that you shouldn't be required support such things as gengtype
working perfectly after each step, if this means that you have to
clutter your patches with code specifically for that and which will
never ever be exercised by gengtype in the trunk.

On 09/09/2010 12:08 PM, Laurynas Biveinis wrote:
> 2010/9/9 Basile Starynkevitch <basile@starynkevitch.net>:
>> In http://gcc.gnu.org/ml/gcc/2010-08/msg00353.html Ian Taylor wrote in
>> a reply to my questions
>>
>>>> An ideal sequence of patches is a series of patches which may be applied
>>>> in order.  At each stage the tree should be buildable.  The patches
>>>> don't have to be independent--they can be applied in a specific
>>>> order--but they should each work.
> 
> I think that you shouldn't be required support such things as gengtype
> working perfectly after each step, if this means that you have to
> clutter your patches with code specifically for that and which will
> never ever be exercised by gengtype in the trunk.

The point is that breaking up the patches is not just for review, it's
also for checking in smaller pieces.  That allows each piece to be
tested, and it helps identify problems more easily later, but for that
to work every piece must result in a working compiler.


Bernd

> 	* Makefile.in:
> 	(REVISION): Always defined.
> 	(version.o): Removed ifdef REVISION_c.
> 	(s-gtype): Pass arguments to build/gengtype program.
> 	(build/version.o): Added building rule.
> 	(build/gengtype$(build_exeext)): Added build/version.o.

Build parts are ok, thanks!

Paolo

On 09/09/2010 08:07 AM, Basile Starynkevitch wrote:
> I still don't understand who has a reviewer hat

Everybody can informally review patches, including you.

Instead, what MAINTAINERS tell you is who approve patches; "maintainers" 
do not need external reviews, "reviewers" do.  Maybe "committers" and 
"maintainers" could be a better choice of words, but that's too late now 
to change it.

There are 2*2 possible scenarios which involve reviews from people not 
in MAINTAINERS:

1) Ralf reviews the patch and finds it is okay.  After I read his 
review, I still have to approve it explicitly if I agree, or reject it 
if I disagree.

2) I approve the patch, but Ralf comes and finds something I missed. 
It's usually good manners on your part to avoid committing the patch 
until I come back and resolve the problem (usually by rejecting the 
patch :).

3) Laurynas comes first and he points out problems in your patch.  The 
actual maintainers/reviewers usually will not reject the patch 
explicitly if they agree with him, but you should assume they do.  Maybe 
they can say "Ok with the changes suggested by Laurynas", in which case 
you can fix the changes and commit the patch.

4) Laurynas points out problems in your patch, the actual 
maintainers/reviewers disagree.  They start a discussion and you'll 
notice it.


To some up, usually you should pay more attention to bad news than to 
good news if you want your patches to go in. :)

Also, for patch series as big as this one you should really, really 
learn quilt or git.  (I wrote my own simplified version of quilt, but I 
was a student when I did...).

Paolo

On Thu, Sep 09, 2010 at 03:02:10PM +0200, Paolo Bonzini wrote:
> On 09/09/2010 08:07 AM, Basile Starynkevitch wrote:
> >I still don't understand who has a reviewer hat
> 
> Everybody can informally review patches, including you.
> 
> Instead, what MAINTAINERS tell you is who approve patches;
> "maintainers" do not need external reviews, "reviewers" do.  Maybe
> "committers" and "maintainers" could be a better choice of words,
> but that's too late now to change it.
> 
> There are 2*2 possible scenarios which involve reviews from people
> not in MAINTAINERS:

Thanks for the explanation!
[....]
> 
> 
> To some up, usually you should pay more attention to bad news than
> to good news if you want your patches to go in. :)

I try hard to follow the bad news.


> 
> Also, for patch series as big as this one you should really, really
> learn quilt or git.  (I wrote my own simplified version of quilt,
> but I was a student when I did...).


I will continue sending the patch series "completed!" but I am not at
all ignoring the comments gotten so far. after I end sending this
"completed!" series, I will send again a "third round" serie of
patches, taking into account every comment recieved so far.

Regarding formatting & indentation issues, Jeremie Salvucci, when he
typed code, had a non GNU-conformant .emacs (while I Basile hopefully
do have a GNU conformant .emacs file).  This explains the many
indentation issues.  But I am the only one to be blamed.  Jeremie was
my intern, and I made the mistake to not noticing that his .emacs was
not conformant to GCC rules, and to not correcting Jeremie's coding
habits while he was typing with me.  So please blame me Basile, not
him Jeremie!

Regarding my next serie of patches - the incoming third round - I
would like very much to get a concrete advice regarding quilt
use. Paolo, what concrete procedure do you suggest? I was thinking of
starting from a more recent trunk, apply each patch I am sending in
this second round "complete!" serie, re-reading them manually,
re-indenting with Emacs with correct setting, and of course re-reading
all the comments we got so far. But I miss how to concretely use quilt
in that case.  Or do you think it is too late to use quilt?

Cheers.

On 09/09/2010 03:28 PM, Basile Starynkevitch wrote:
> Regarding my next serie of patches - the incoming third round - I
> would like very much to get a concrete advice regarding quilt
> use. Paolo, what concrete procedure do you suggest? I was thinking of
> starting from a more recent trunk, apply each patch I am sending in
> this second round "complete!" serie, re-reading them manually,
> re-indenting with Emacs with correct setting, and of course re-reading
> all the comments we got so far. But I miss how to concretely use quilt
> in that case.  Or do you think it is too late to use quilt?

No idea exactly of how to use quilt because I don't use it, but it looks 
like the right plan.

The main problem is that after you fix indentation for patch 1, parts of 
patch 2 will not apply.  So you need some patience, but you don't lack 
it.  Emacs diff-mode helps a lot in this respect.

Paolo

On Thu, 9 Sep 2010, Paolo Bonzini wrote:

> Also, for patch series as big as this one you should really, really learn
> quilt or git.  (I wrote my own simplified version of quilt, but I was a
> student when I did...).

Or, for the future, submit minimal self-contained patches as you go along 
as far as possible rather than building up a huge change you then need to 
split up.  My multilibs / option handling series has more than 40 patches 
so far (the exact number depending on how you count), submitted and 
reviewed one at a time rather than building up a branch and then trying to 
split it, no quilt, no git.

Whenever in the course of a larger development you find yourself fixing an 
issue present on trunk, or cleaning up in a way also applicable to trunk, 
you should at least consider stopping making that change in the context of 
your larger development, preparing and submitting a trunk version of that 
fix instead, then merging trunk into the sources used for your larger 
development to get the fix into them.

On Thu, Sep 9, 2010 at 09:42, Joseph S. Myers <joseph@codesourcery.com> wrote:

> Whenever in the course of a larger development you find yourself fixing an
> issue present on trunk, or cleaning up in a way also applicable to trunk,
> you should at least consider stopping making that change in the context of
> your larger development, preparing and submitting a trunk version of that
> fix instead, then merging trunk into the sources used for your larger
> development to get the fix into them.

Agreed.  Even, if you are working on a long lived branch, take
advantage of all the stage 1s to flush out all those dozen little
cleanups that may have accumulated during the project.  This makes
your main contribution that much easier to separate from the fluff.


Diego.