From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21350 invoked by alias); 30 Sep 2003 22:54:00 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 21337 invoked from network); 30 Sep 2003 22:53:59 -0000 Received: from unknown (HELO hawaii.kealia.com) (209.3.10.89) by sources.redhat.com with SMTP; 30 Sep 2003 22:53:59 -0000 Received: by hawaii.kealia.com (Postfix, from userid 2049) id 3ED44CB28; Tue, 30 Sep 2003 15:53:59 -0700 (PDT) To: Jim Blandy Cc: gdb-patches@sources.redhat.com, Elena Zannoni Subject: Re: [rfa] add 'parent' field to struct die_info References: From: David Carlton Date: Tue, 30 Sep 2003 22:54:00 -0000 In-Reply-To: (Jim Blandy's message of "30 Sep 2003 17:09:38 -0500") Message-ID: User-Agent: Gnus/5.1002 (Gnus v5.10.2) XEmacs/21.4 (Rational FORTRAN, linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-09/txt/msg00671.txt.bz2 On 30 Sep 2003 17:09:38 -0500, Jim Blandy 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