Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Saturday, August 05 2017, Simon Marchi wrote:

> On 2017-08-05 01:03, Sergio Durigan Junior wrote:
>> So, I've been thinking about this implementation over the last few
>> days,
>> but it's still a bit confuse to me.  My C++-foo is not so good as
>> yours,
>> so maybe you can give me a hand.
>>
>> From what I understood initially, your intention was to make a new
>> class
>> that inherited from std::vector but overloaded/implemented methods to
>> mimic what a std::set would do.  However, after reading a bit, it
>> doesn't seem like a good idea to inherit from std::vector (or any std
>> containers).  Which made me realize that maybe you are talking about
>> creating a class that encapsulates a std::vector, without inheriting
>> from it, and that provided the regular .push_back, .insert, .empty,
>> etc., methods, but in an enhanced way in order to e.g. prevent the
>> insert of duplicated elements, which is one of the things we miss from
>> std::set.
>
> Do you mean "... we miss from std::vector"?

I meant that we miss a few features that std::set has, and we'd like to
have them implemented alongside with what std::vector offers.

> I also read about inheriting STL containers.  The danger is that they
> don't have virtual destructors.  So let's say we have gdb::set_vector
> inheriting std::vector and try to delete a set_vector instance through
> a vector pointer, the set_vector destructor won't be called.  If
> set_vector is simply adding some methods to the interface (no data
> members, no user defined destructor), I don't know if this is really a
> problem.

Right, that's the main issue I've found, too.  As I was/am not sure this
is really an issue for this specific case, I decided to consult you
first.

> But now that I think of it, if we want to make sure this object
> guarantees the properties of a set (like no duplicate elements), we
> would have to hide most of the vector interface and only expose a
> set-like interface.  Otherwise, it would be possible to call push_back
> and insert a duplicate element.  Given that, I'd lean towards creating
> a class that includes a vector and exposes a set-like interface,
> rather than inheriting.

Hm, OK, it's nice to know that I was leaning towards a solution that
crossed your mind as well.

>>
>> Am I in the right direction here?  Also, I started to think...  I don't
>> envision the user setting thousands of user-set environment variables,
>> so *maybe* using std::set would be OK-ish for our use case scenario.  I
>> don't know.  While I understand the concern about premature
>> pessimization, I'm also not a fan of complicating the implementation
>> just for a little bit of optimization either.
>>
>> WDYT?
>
> Actually, if we expected the user to set thousands of environment
> variables and needed it to be fast to set and unset variables, it
> would be good to use std::set because of the O(log(N))
> lookups/insertions/removals, which matters more when you have a lot of
> elements.  But when you just have a few elements, the constant cost is
> more significant.  A vector-based set would have O(N) complexity for
> these operations (at least for insertions and removals, for lookups it
> depends if it is sorted), which would be bad if we had thousands of
> elements.  But since we expect to have just a few, it would likely be
> faster than std::set's constant cost.

You mean std::vector's constant cost, right?

> But I agree with you that the important thing now is to get the
> algorithm/functionality right.  The typical use case is to have at the
> maximum a few inferiors, which may have at the maximum a few
> environment variables defined.  The defined environment variables are
> only used when doing a "run", which doesn't happen often.  So in the
> end, I don't think it would make a perceptible difference in memory
> usage or execution time to use an std::set.  If it helps to make the
> code significantly simpler, then I think it's worth it.  And we can
> always revisit it later by making a drop-in replacement for std::set.

It indeed helps make the code simpler if I use std::set, which is what
I'm using in the last version of the patch.  I'm glad that you
understood and agreed with my point, so I'll go ahead and submit this
version soon (I'll fix the testcase problem first, and run more tests to
make sure everything is OK).

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/