From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 278953857C79 for ; Fri, 18 Sep 2020 05:49:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 278953857C79 Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-21-9mSDYVyvNce5eRdVycdQpQ-1; Fri, 18 Sep 2020 01:49:40 -0400 X-MC-Unique: 9mSDYVyvNce5eRdVycdQpQ-1 Received: by mail-wm1-f71.google.com with SMTP id t10so1085499wmi.9 for ; Thu, 17 Sep 2020 22:49:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=dousG6t1yQEfgxf8xkj9EFlFiO02giWYlkPVVg3++J8=; b=dAsizTBRl6R5zPzKwc8jxukucq2VdJm4D6/6iyYKtsQEhMC5pkMGTZQFffQtBcUHF6 i5oBnInt4FrX9C2xFX3nwEIM71WTP5jU/8Pg3ffSKHr7Q4r4+zycgNjWChfeGzzRe63R QHboc/OewmptY7qqboEnGcm4O5PSJf5jhowlrEPI4vdkmM6Es7jM1P6syKoFNLRdjSnu iuakgm+IQ3dhg/EVyXrkT3M+MPXyUBymk8oCZi/s/VKwmHUsekw5o3dvvIH5tpu8QlO/ S+ixAMWjG7xG4iOVP7rrx7y9e6jKpMpTGO4AXcOx8I4QPdKYlgagLWjUjpjTh1qmyPy9 TmcQ== X-Gm-Message-State: AOAM532F67gNDzSXXrT6jOEtaO5+qZJLw1QNSwOYx5HHfuMhylJ9S3pA D71g6ylDnx+XEVY+c/zD4qxkWryGssaWGQYLIf5kpOE3yo+I9oLneU1lznfVF3tKxvLWpsrQqt4 YC/wWxCsQzlrk33NqwA== X-Received: by 2002:a1c:1d52:: with SMTP id d79mr14477107wmd.82.1600408179550; Thu, 17 Sep 2020 22:49:39 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyNW7FDR1VVTwAxWO9VCOIUoYxAGrnEdHavZDfIc0s+z56ikPpJHrbiEGqd7b2IvXOb2YVUAg== X-Received: by 2002:a1c:1d52:: with SMTP id d79mr14477089wmd.82.1600408179311; Thu, 17 Sep 2020 22:49:39 -0700 (PDT) Received: from abulafia.quesejoda.com ([95.169.225.22]) by smtp.gmail.com with ESMTPSA id k6sm2810999wmf.30.2020.09.17.22.49.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Sep 2020 22:49:38 -0700 (PDT) Subject: Re: [PATCH] irange_pool class To: David Malcolm , gcc-patches , Andrew MacLeod References: <8b999eaf506d993b00edda3419dd82d6ad8df170.camel@redhat.com> From: Aldy Hernandez Message-ID: <3b5e7d6c-5bc2-440f-d9b8-ad6c24707f01@redhat.com> Date: Fri, 18 Sep 2020 07:49:37 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <8b999eaf506d993b00edda3419dd82d6ad8df170.camel@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Sep 2020 05:49:44 -0000 On 9/18/20 3:43 AM, David Malcolm wrote: > On Thu, 2020-09-17 at 12:36 +0200, Aldy Hernandez via Gcc-patches > wrote: >> This is the irange storage class. It is used to allocate the >> minimum >> amount of storage needed for a given irange. Storage is >> automatically >> freed at destruction. >> >> It is meant for long term storage, as opposed to int_range_max which >> is >> meant for intermediate temporary results on the stack. >> >> The general gist is: >> >> irange_pool pool; >> >> // Allocate an irange of 5 sub-ranges. >> irange *p = pool.allocate (5); >> >> // Allocate an irange of 3 sub-ranges. >> irange *q = pool.allocate (3); >> >> // Allocate an irange with as many sub-ranges as are currently >> // used in "some_other_range". >> irange *r = pool.allocate (some_other_range); > > FWIW my first thoughts reading this example were - "how do I deallocate > these iranges?" and "do I need to call pool.deallocate on them, or is > that done for me by the irange dtor?" Thus the description front and center of the header file: "Storage is automatically freed at destruction..." > > I think of a "pool allocator" as something that makes a small number of > large allocation under the covers, and then uses that to serve large > numbers of fixed sized small allocations and deallocations with O(1) > using a free list. Ah, I didn't know pool had a different meaning. > > [...] > >> +// This is the irange storage class. It is used to allocate the >> +// minimum amount of storage needed for a given irange. Storage is >> +// automatically freed at destruction. > > "at destruction" of what object - the irange or the irange_pool? > Reading the code, it turns out to be "at destruction of the > irange_pool", and it turns out that irange_pool is an obstack under the > covers (also called a "bump allocator") and thus, I believe, the > lifetime of the irange instances is that of the storage instance. The sentence is talking about the storage class, so I thought it was obvious that the destruction we talk about is the storage class itself. I suppose if it isn't clear we could say: "Storage is automatically freed at destruction of the storage class." > > I think it would be clearer to name this "irange_obstack", or somesuch. I'd prefer something more generic. We don't want to tie the name of the allocator to the underlying implementation. What if we later change to malloc? We'd have to change the name to irange_malloc. irange_allocator? Or is there something more generically appropriate here? > >> +// >> +// It is meant for long term storage, as opposed to int_range_max >> +// which is meant for intermediate temporary results on the stack. >> + >> +class irange_pool >> +{ >> +public: >> + irange_pool (); >> + ~irange_pool (); >> + // Return a new range with NUM_PAIRS. >> + irange *allocate (unsigned num_pairs); >> + // Return a copy of SRC with the minimum amount of sub-ranges >> needed >> + // to represent it. >> + irange *allocate (const irange &src); >> +private: >> + struct obstack irange_obstack; > > ...and thus to rename this field to "m_obstack" or similar. Will do. Thanks. Aldy