Re: [PATCH 1/3] gdb: store internalvars in an std::vector

[Simon Marchi <simark@simark.ca>]

On 2/14/23 05:54, Lancelot SIX via Gdb-patches wrote:
> 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<internalval> 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<std::string, internalvar> 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.

I had not thought of that, but I think it's a good idea.  Having the
name stored twice is not an issue, I think.

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

I don't think it can really happen today anyway.
create_internalvar_type_lazy is only used to create variable known in
advance, for which we know there are no clashes.  And through
lookup_internalvar, create_internalvar is only called after seeing that
there is no variable by that name.  So, I think we'll be fine with a
map.

Simon