Re: [RFC] Alternate approach to keeping convenience variables

[Daniel Jacobowitz <drow at false dot org>]

Meta-issue: When you're reviewing a patch, especially from a peer,
please keep courtesy in mind.  I value your comments and will honor any
suggestions or objections; that doesn't mean that I need your
permission to do things, or that I want to be instructed to fix
problems with pre-existing code.

Especially true when I don't agree that the usage is problematic (e.g.
the use of a macro for TYPE_ZALLOC, paralleling TYPE_ALLOC).

It's the difference between offering comments and advice, and giving
orders.  I will listen to your advice; I will not accept your commands.

I've edited out the remainder of my annoyed comments from this version.

On Fri, Dec 09, 2005 at 03:13:29PM -0800, Jim Blandy wrote:
> I don't understand what the tracepoint.c change has to do with the
> rest of the patch.

Take another look at the experiment in my message.  With the patch
applied, the initially created convenience variables are no longer
discarded by loading the initial objfile.  Either tracepoint.c
has to change, or the output of "show convenience" in default.exp does.

> It would be nice if the comments for hashtab_obstack_allocate and the
> dummy free actually explained what sort of DATA argument they expect.

I've taken care of that locally.

> copy_type_recursive undoes the sharing given a bunch of 'struct type'
> objects all referring to a given 'struct main_type' object.  You could
> just stick both kinds of pointers in the type hash, at the cost of
> some static typing.

We could do this.  It makes the initialization rather more complicated;
we have to bypass alloc_type in some cases, and we can't use
alloc_type_instance because it expects the old type (there's no pointer
from the main_type back to the chain).

Do you think this is worthwhile?  If so I'll try to do it.

>  And it doesn't preserve 'pointer_type',
> 'reference_type', or 'chain' groupings.

That was a deliberate choice, to (in this context) reduce memory usage.
The set of convenience variables will be relatively small.  There's no
point in eagerly copying either the cv-chains, or the
pointer-to/reference-to chains, if they aren't used.  Most of the time
I expect that they won't be; do you expect otherwise?  There's no
straightforward way to non-eagerly preserve the chains.

> Go ahead and expand the comment in copy_type_recursive to spell out
> why it is *necessary* to add the type pair to the hash table before
> the type is completely constructed.  Don't just point out that we do
> it where we do it.

This is standard for any hash-table based tree walker...

> preserve_values also needs a comment indicating that it's meant to be
> called only when we're about to free the given objfile, which ensures
> that we never make more than one copy of a given type.

That's unnecessary; you could call it multiple times if you wanted to
and it would do the right thing (i.e. nothing).  It only looks for
types associated with the objfile to be discarded, and it replaces them
all.

-- 
Daniel Jacobowitz
CodeSourcery, LLC