Re: [rfa] add 'parent' field to struct die_info

[David Carlton <carlton@kealia.com>]

On 30 Sep 2003 17:09:38 -0500, Jim Blandy <jimb@redhat.com> said:

> Looks great --- please commit.

Thanks, done.

> Adding the parent pointer is great.  But I also really appreciate the
> child/sibling rearrangement... the way it stands is really confusing,
> and I think this is much more intuitive.

That was my attitude, too: before, we were too closely tied to the
data structure that the debug info was originally stored in, for no
good reason.

> It doesn't seem to me like the 'abbrev' field of 'struct die_info'
> is being used for anything very important; removing that would bring
> 'struct die_info' back to the size it has now.

Good point.

> I wonder if there isn't a clearer way to read the trees.  The
> read_die_and_children / read_die_and_siblings pair is harder for me
> to understand than it seems like it should be.  But the burden is on
> me to show that there's a better way...

I would believe that there's a better way out there.  I remember
making some mistakes when first writing the code; and when I went back
and looked at it to generate this patch, I'd forgotten about the
invariants of the various functions, so I was confused again.  (Which
is why I added comments at the starts of the functions about what
fields were being set to NULL instead of being filled in correctly.)

At some point in our copious free time, we can revisit the issue.
Hmm.  I guess one thing that's annoying is the existence and number of
out parameters: for example, read_full_die has a return value and
two(!) out parameters now.  The thing is, info_ptr always causes
trouble this way: it's an out parameter for read_die_and_XXX, too, and
similarly when reading partial dies.  And it's not like we ever need
to remember the old values of info_ptr: we're reading the debug info
sequentially, and info_ptr just stores our current position.  So
keeping it around as a parameter doesn't make any more sense than,
say, designing a file structure where you always have to pass in (and
have returned back to you after the read) the current read position.

So if we were doing this in an object-oriented language, I guess we
could make a DWARF 2 parser class that these various functions would
be methods of; then info_ptr would be a data member of that class, and
all of the methods would update that, getting rid of an input and an
output parameter.  (Not to mention, say, abfd or cu_header: those
could be const data members.)  That would be a nice cleanup.
Unfortunately, it still leaves read_full_die with one output
parameter, namely has_children.  And there, I suspect, my structure is
simply wrong: read_full_die and read_die_and_children should probably
be merged somehow, or split into two functions along different lines.
So probably I stuck too closely to the original structure of the code:
I mostly restricted myself to modifying read_comp_unit, but I should
have modified read_full_die more as well.

Well, it's something to think about: certainly this patch won't get in
the way of such a further cleanup.

David Carlton
carlton@kealia.com