Re: [RFC] Alternate approach to keeping convenience variables

[Jim Blandy <jimb at red-bean dot com>]

On 12/9/05, Daniel Jacobowitz <drow@false.org> wrote:
> 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.

Whoa.  Being bossy wasn't on my mind.  I did write imperative
sentences, but only because it was terser than writing "I think", "How
about", etc. each time.  If it came across as something other than
collegial, I'll certainly avoid that in the future.

> 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).

Well, you're adding a new usage.  I think macros are fine for things
where they offer a brevity that you can't get with other kinds of
abstractions, but this seems like a case where there isn't much
advantage; and you lose type checking, stepping ability, etc.

> 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.

Okay, I see.  I think default.exp should be changed; preserving the
values of those variables seems like a feature, not a bug.

> > 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.

... I'm not sure it's worthwhile.

> >  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.

No, I see what you mean with this.

>
> > 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...

Yes, but the code will also be read by people trying to learn the
algorithm, not just by people who already know it.  If you like, I'll
write the comment.

> > 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.

Maybe I'm misunderstanding.  If you call it multiple times on the same
objfile, and there are new convenience variables whose types come from
that objfile each time, it will make fresh copies of all the new
variables' types, rather than sharing them with those of values
preserved by prior calls, because we allocate a fresh hash table each
time preserve_values is called.  Isn't that right?  I'm not sure this
is important enough to worry about; it's just a facet of the behavior
that I thought was worth explaining.