From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 9F1853858D1E for ; Tue, 14 Feb 2023 10:54:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9F1853858D1E Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from octopus (unknown [IPv6:2a02:390:9086:0:e9e7:8861:a4b1:1462]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 7C1EE80A0D; Tue, 14 Feb 2023 10:54:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1676372093; bh=StybTlT9WwX8QlIUUFJs0PBDAt2jZhq9keKG/bxdytM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=YGJzB+XqUAPQRPx0QJOPuW9DeToeAmoUysmdpo3EH2UwGITKumDQeTwSy8nay4OYs jSoSHXiVKDQLwUFrqYeaqIadwGiaeC08giX6GeFX0RmckUA51WtjBX76EYZ7a1fTSB FLrHdgzqFoKATesbXr+LybvTTN7zoGiKItiiaStn1q0w4Y/ogW9pYq2Ot3Y4y58XJB nG+0zMW3XP48nagY8f0P0XAXTj247UHEmLw8kEL20pa8W2yU6RKNblkjZBtN9VTfp+ nzRbHj5La9pd3LjjmEGLx69TijzPL6ZW/J4rsnodWp0kmK0z+um5Z4q+3BvYw1fpPZ 6Ftdq/KBfR9Iw== Date: Tue, 14 Feb 2023 10:54:46 +0000 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] gdb: store internalvars in an std::vector Message-ID: <20230214105446.desfcw5xz5wpiqbt@octopus> References: <20230214042139.73638-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230214042139.73638-1-simon.marchi@efficios.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 14 Feb 2023 10:54:53 +0000 (UTC) X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Feb 13, 2023 at 11:21:37PM -0500, Simon Marchi via Gdb-patches wrote: > Change the storage of internalvars to an std::vector of unique pointers > to internalval. This helps automate memory management, and will help > keep internalvars sorted in a subsequent patch. > > I initially tried to use an std::vector initially, but some > parts of the code need for the addresses of internalvars to be stable. > Hi Simon, As the end-goal is to have an order, did you consider using a container which enforces it like a std::map? For small number of objects (for some definition of small…) vector is usually more efficient but as here you hold pointers, I am not sure the vector brings a huge benefit. You could use a std::map as the internalvar’s address will remain stable. No need to have a unique_ptr. One downside of the std::map is that you might end up with the name stored twice (once as key, and maybe still once in the internalvar object). std::set can also be used, but with other downsides. The main difference with vector can come if we had 2 internal variables with the same name. The current code does not prevent this AFAICT, but as one of the vars would effectively shadow the other, I am not sure this is a case we want to support anyway. Best, Lancelot. > Change-Id: I1fca7e7877cc984a3a3432c7639d45e68d437241 > --- > gdb/value.c | 32 +++++++++++++------------------- > 1 file changed, 13 insertions(+), 19 deletions(-) > > diff --git a/gdb/value.c b/gdb/value.c > index 7873aeb9558e..e884913abe5a 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -1829,7 +1829,6 @@ union internalvar_data > > struct internalvar > { > - struct internalvar *next; > char *name; > > /* We support various different kinds of content of an internal variable. > @@ -1841,7 +1840,9 @@ struct internalvar > union internalvar_data u; > }; > > -static struct internalvar *internalvars; > +using internalvar_up = std::unique_ptr; > + > +static std::vector internalvars; > > /* If the variable does not already exist create it and give it the > value given. If no value is given then the default is zero. */ > @@ -1891,11 +1892,9 @@ init_if_undefined_command (const char* args, int from_tty) > struct internalvar * > lookup_only_internalvar (const char *name) > { > - struct internalvar *var; > - > - for (var = internalvars; var; var = var->next) > + for (internalvar_up &var : internalvars) > if (strcmp (var->name, name) == 0) > - return var; > + return var.get (); > > return NULL; > } > @@ -1906,12 +1905,11 @@ lookup_only_internalvar (const char *name) > void > complete_internalvar (completion_tracker &tracker, const char *name) > { > - struct internalvar *var; > int len; > > len = strlen (name); > > - for (var = internalvars; var; var = var->next) > + for (internalvar_up &var : internalvars) > if (strncmp (var->name, name, len) == 0) > tracker.add_completion (make_unique_xstrdup (var->name)); > } > @@ -1922,12 +1920,12 @@ complete_internalvar (completion_tracker &tracker, const char *name) > struct internalvar * > create_internalvar (const char *name) > { > - struct internalvar *var = XNEW (struct internalvar); > + internalvars.emplace_back (new internalvar); > + internalvar *var = internalvars.back ().get (); > > var->name = xstrdup (name); > var->kind = INTERNALVAR_VOID; > - var->next = internalvars; > - internalvars = var; > + > return var; > } > > @@ -2412,8 +2410,6 @@ preserve_one_varobj (struct varobj *varobj, struct objfile *objfile, > void > preserve_values (struct objfile *objfile) > { > - struct internalvar *var; > - > /* Create the hash table. We allocate on the objfile's obstack, since > it is soon to be deleted. */ > htab_up copied_types = create_copied_types_hash (); > @@ -2421,8 +2417,8 @@ preserve_values (struct objfile *objfile) > for (const value_ref_ptr &item : value_history) > item->preserve (objfile, copied_types.get ()); > > - for (var = internalvars; var; var = var->next) > - preserve_one_internalvar (var, objfile, copied_types.get ()); > + for (internalvar_up &var : internalvars) > + preserve_one_internalvar (var.get (), objfile, copied_types.get ()); > > /* For the remaining varobj, check that none has type owned by OBJFILE. */ > all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj) > @@ -2438,14 +2434,12 @@ static void > show_convenience (const char *ignore, int from_tty) > { > struct gdbarch *gdbarch = get_current_arch (); > - struct internalvar *var; > int varseen = 0; > struct value_print_options opts; > > get_user_print_options (&opts); > - for (var = internalvars; var; var = var->next) > + for (internalvar_up &var : internalvars) > { > - > if (!varseen) > { > varseen = 1; > @@ -2456,7 +2450,7 @@ show_convenience (const char *ignore, int from_tty) > { > struct value *val; > > - val = value_of_internalvar (gdbarch, var); > + val = value_of_internalvar (gdbarch, var.get ()); > value_print (val, gdb_stdout, &opts); > } > catch (const gdb_exception_error &ex) > -- > 2.39.1 >