From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32705 invoked by alias); 26 Dec 2006 16:10:52 -0000 Received: (qmail 32694 invoked by uid 22791); 26 Dec 2006 16:10:51 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Tue, 26 Dec 2006 16:10:47 +0000 Received: from drow by nevyn.them.org with local (Exim 4.63) (envelope-from ) id 1GzEsz-0004aG-PO; Tue, 26 Dec 2006 11:10:41 -0500 Date: Tue, 26 Dec 2006 16:10:00 -0000 From: Daniel Jacobowitz To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Cleanup varobj children handling Message-ID: <20061226161041.GD16188@nevyn.them.org> Mail-Followup-To: Vladimir Prus , gdb-patches@sources.redhat.com References: <200612082300.06688.ghost@cs.msu.su> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.13 (2006-08-11) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-12/txt/msg00329.txt.bz2 Thanks for doing this, I think it's a good cleanup. To answer Nick's questions: Vlad and I hadn't talked about this change, but I agree with him. The FIXME about should use vlists goes along with this one that Vlad deleted: /* FIXME: This should be a generic add to list */ vec.h is basically the generic list structure the code was asking for. On Thu, Dec 21, 2006 at 06:25:04PM +0300, Vladimir Prus wrote: > #include "varobj.h" > +#include "vec.h" > + Extra blank line. > - /* A list of this object's children */ > - struct varobj_child *children; > + /* Children of this object. */ > + VEC (varobj_p) *children; I've been omitting the space before the parenthesis for VEC, since it's a macro creating a type and syntactically very different from a function call. I can't remember where I got that habit though! Either way is fine. > if (var->num_children == -1) > - var->num_children = number_of_children (var); > + { > + var->num_children = number_of_children (var); > + } Braces :-) > + /* If we're called when the list of children is not yet initialized, > + allocated enough elements in it. */ > + while (VEC_length (varobj_p, var->children) < var->num_children) > + VEC_safe_push (varobj_p, var->children, NULL); You need the NULLs initialized here, right? Otherwise you could use grow instead, but that doesn't initialize. > @@ -721,13 +701,18 @@ varobj_list_children (struct varobj *var > /* Mark as the end in case we bail out */ > *((*childlist) + i) = NULL; > > - /* check if child exists, if not create */ > - name = name_of_child (var, i); > - child = child_exists (var, name); > - if (child == NULL) > - child = create_child (var, i, name); > + varobj_p *existing = VEC_address (varobj_p, var->children) + i; Please declare existing at the top of the block, instead of in the middle; we only require C90 compatibility. Do you really need to use VEC_address here? You could use VEC_index to read it and VEC_replace to modify it; I find that easier to read than a pointer into the vec's storage. > /* If this is a "use_selected_frame" varobj, and its type has changed, > them note that it's changed. */ > if (type_changed) > { > - vpush (&result, *varp); > - changed++; > + VEC_safe_push (varobj_p, result, *varp); > } Braces again :-) A few other places too. > + for (i = 0; i < VEC_length (varobj_p, (*varp)->children); ++i) > + { > + VEC_safe_push (varobj_p, stack, > + VEC_index (varobj_p, (*varp)->children, i)); > } Here and elsewhere, VEC_iterate avoids having to call VEC_index - that's what it's for. > - /* Copy from result stack to list */ > - vleft = changed; > - *cv = vpop (&result); > - while ((*cv != NULL) && (vleft > 0)) > - { > - vleft--; > - cv++; > - *cv = vpop (&result); > - } > - if (vleft) > - warning (_("varobj_update: assertion failed - vleft <> 0")); > + cv = *changelist; > > - if (changed > 1) > + for (i = 0; i < changed; ++i) > { > - /* Now we revert the order. */ > - for (i = 0; i < changed; i++) > - *(*changelist + i) = *(templist + changed - 1 - i); > - *(*changelist + changed) = NULL; > + *cv = VEC_index (varobj_p, result, i); > + gdb_assert (*cv != NULL); > + ++cv; > } > + *cv = 0; > > if (type_changed) > return -2; It looks to me as if the old code was somewhat careful about the ordering, and the old ordering was a bit nicer. But it also looks to me as if you're preserving the same ordering - vpop takes what would be the "last" item in the vstack. So why does this patch reverse the order for the testsuite? > @@ -1227,7 +1179,7 @@ delete_variable_1 (struct cpstack **resu > discarding the list afterwards */ > if ((remove_from_parent_p) && (var->parent != NULL)) > { > - remove_child_from_parent (var->parent, var); > + VEC_replace (varobj_p, var->parent->children, var->index, NULL); > } > > if (var->obj_name != NULL) I think that this will cause crashes in -var-update, since that walks the list of children without checking for NULL. If I'm right, you probably want to add NULL checks rather than use VEC_ordered_remove, so that the indexes still tell you which child it is? -- Daniel Jacobowitz CodeSourcery