From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19354 invoked by alias); 12 Aug 2017 04:34:09 -0000 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 Received: (qmail 19190 invoked by uid 89); 12 Aug 2017 04:33:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=confuse, encapsulates X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 12 Aug 2017 04:33:51 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F00A5C0C2E4D; Sat, 12 Aug 2017 04:33:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F00A5C0C2E4D Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A7FBA5D765; Sat, 12 Aug 2017 04:33:47 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Pedro Alves , Eli Zaretskii Subject: Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior References: <20170629194106.23070-1-sergiodj@redhat.com> <20170727033531.23066-1-sergiodj@redhat.com> <87shhc9ffa.fsf@redhat.com> <291aec4022448984a38891ddbccf08e1@polymtl.ca> <87mv7f7x6t.fsf@redhat.com> <7be6bdecb0da90c8b2efb550fc017a5e@polymtl.ca> Date: Sat, 12 Aug 2017 04:34:00 -0000 In-Reply-To: <7be6bdecb0da90c8b2efb550fc017a5e@polymtl.ca> (Simon Marchi's message of "Sat, 05 Aug 2017 20:14:20 +0200") Message-ID: <8760dt76c4.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00262.txt.bz2 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/