Re: [PATCH] gc-improv merge (1/n): new libiberty interfaces

[Basile Starynkevitch <basile at starynkevitch dot net>]

On Wed, May 05, 2010 at 03:36:34PM +0200, Laurynas Biveinis wrote:
> 
> splay_tree_new_with_separate_allocators
> htab_create_alloc_with_separate_allocators
> 
> The names are quite clumsy, any better suggestions?

I tend to like long & descriptive names, so I am happy with your
proposed names (as a general hint, I believe all our new global names
should be longer than the habits ten or twenty years ago). However, if
some people like shorter names, we might consider
splay_tree_new_sep_alloc & htab_create_sep_alloc or something
similar. My personal opinion is that your proposed names
splay_tree_new_with_separate_allocators &
htab_create_alloc_with_separate_allocators are in fact better!


> +htab_create_alloc_with_separate_allocators (size_t size, htab_hash hash_f,
> +					    htab_eq eq_f, htab_del del_f,
> +					    htab_alloc alloc_tab_f,
> +					    htab_alloc alloc_f,
> +					    htab_free free_f)

I like that kind of declaration, and I *really* dislike a lot
declaring the same function without naming its formal argument. But in
your proposed include/hashtab.h you are just writing:

+extern htab_t  htab_create_alloc_with_separate_allocators(size_t, htab_hash,
+							  htab_eq, htab_del,
+							  htab_alloc,
+							  htab_alloc,
+							  htab_free);

I really think that you should leave everywhere the formal arguments
names for documentation purposes. (and BTW, there might be a missing
space before the left parenthesis above). So please add the formal
arguments name (like size, hash_f, eq_f... above) or at least explain
why you don't want to add them.

At last, I would like some descriptive comments if possible. I am not
entirely sure to understand exactly what you want to do -especially
without formal arguments names, and I believe a newscomer also could
be lost. Perhaps also the htab_create_alloc function should be
documented as deprecated (in hashtab.h) if you believe it is becoming
deprecated.

Still, I like a lot your work (and hope that your patch will be
accepted, with the small corrections I am suggesting).

I also don't have a clear picture of when and if your gc-improv work
would be entirely [or sufficiently] merged into the trunk. I am really
scared of merging the trunk into the MELT branch before your gc-improv
is done (because the GC is perhaps the piece of code MELT depends upon
the most. Fortunately, most of the dependence is in melt-runtime.c,
not in the MELT C code generator, since the MELT translator's
generated C code only allocates its data thru the melt-runtime.h
API...).

I hope your flu is gone.

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 mines, sont seulement les miennes} ***