Re: patch ping^3: gengtype: improved version for plugin support.

[Duncan Sands <baldrick at free dot fr>]

Hi Basile,

> The bug is that gt_files entries are not plain strings, they should be 
> prefixed by a word for the lang_bitmap.
>
> The attached patch (to trunk r151258) is the same patch with this bug 
> corrected. I am also attaching the gcc/ChangeLog entry.

thanks for doing this.  I also did a patch, and the funny thing is that
it was really exactly the same as yours (use of calloc to zero the
bitmap and the string terminator etc), except I used memcpy rather than
strcpy.

Some comments below:

> +	      /* Each added entry in gt_files should have additional
> +		 space for its lang_bitmap before.  */

I think it's important to note that the lang_bitmap is set to all
zero (it won't be changed later) since this is a policy decision.
How about this comment instead:

  /* Place an all zero lang_bitmap before the plugin file name.  */

> +	      char* plugent =
> +		(char*) xcalloc (1, plugfilen+1 + sizeof (lang_bitmap));

Missing space before +1.

> +	      strcpy(plugent, plugin_files[i]);

This can be memcpy (more efficient, not that it's important) since the
null terminator was taken care of by xcalloc.

I also have some general comments on the rest of your patch.  First off,
I think you should split it up into multiple small patches, each fixing
one thing, rather than submitting a patch that fixes lots of things at
once.  Then it is much more likely to be reviewed, since each piece will
be more easily digestible.

Secondly, your patch clearly solves some serious problems with gengtype
and plugins.  With it I get a usable header file generated, without it
I do not.  When I first read about your patch in
  http://gcc.gnu.org/ml/gcc-patches/2009-07/msg00507.html
I thought it was essentially cosmetic, since it doesn't mention that
it solves the following bug.  Consider the following file gtest.c:

#include "config.h"
#include "system.h"
#include "coretypes.h"
#include "target.h"
#include "tree.h"

struct GTY(()) tree_llvm_map {
  struct tree_map_base base;
  const void * GTY((skip)) val;
};

static GTY ((if_marked ("tree_map_base_marked_p"),
             param_is (struct tree_llvm_map)))
  htab_t llvm_cache;

#include "gt-gtest.h"

If I run gengtype on this without your patch then I get a gt-gtest.h
which uses gt_ggc_m_13tree_llvm_map, but doesn't define it; likewise
for several other functions.  If I use gengtype with your patch then
all these functions are defined (plus a lot more) and I can compile
gtest.c.  So for me gengtype is unusable without your fixes.  I think
you should more clearly explain what serious bugs like this you are
fixing.

Finally, I notice that you introduced trailing whitespace on a few
lines.

Thanks for working on this!

Best wishes,

Duncan.