From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x331.google.com (mail-wm1-x331.google.com [IPv6:2a00:1450:4864:20::331]) by sourceware.org (Postfix) with ESMTPS id DEFE23857C5F; Wed, 29 Jul 2020 09:35:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DEFE23857C5F Received: by mail-wm1-x331.google.com with SMTP id k20so2237564wmi.5; Wed, 29 Jul 2020 02:35:43 -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:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=gQWaxGyxeWnA1Ti6O+0xFefs3evedO+JMlVj6S5KB/8=; b=l5AUXAk61VHUuVlBAbh9m+8wTrFF0+ZvO1fPRbCY7oa1VoJbUT1bgh7Vr6VAFbebiy o2FzTX9v3C6Y63edg4S4Ox3o0CoL9l32Tz+LUj6jCegnWsu6gAvGyvusOXgTuFyTxEk/ F8LHBHsWkX1wtl336kVEDZ4bmE6pNha/qMMJ9BvgN3bruCb89XfrwRGPFKPIMEu8fceD 0nD+llO1LzbYWYAOcR0q+iWn1rVmH0jxmEEa18b908YElUf9cdJjxIIcl/d3DHvvJZQX D8STNkdbdfbQ51ufOWBDohz1MszCSFeNMSS9CWMPf4Fpi1uABCATsojrs3JB3shkwuvl DDOg== X-Gm-Message-State: AOAM530lj1cDW8SjG53Lk8RHxPlIC7yv9M1ytVClLyPBU+frsQaHal7N 7IGnFUAIMg6315VCBsrrjrc7cwiDSmQ= X-Google-Smtp-Source: ABdhPJxK7jj4TRezmMvfcSN1LW4Xr4sOl3UHYftQOd/JHeMbisgS4KkFCCVWuQt7UBjf5WU4HFJKlg== X-Received: by 2002:a1c:67d4:: with SMTP id b203mr7801410wmc.8.1596015342750; Wed, 29 Jul 2020 02:35:42 -0700 (PDT) Received: from [192.168.42.161] ([37.167.158.175]) by smtp.googlemail.com with ESMTPSA id o4sm3677537wmc.13.2020.07.29.02.35.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Jul 2020 02:35:41 -0700 (PDT) Subject: Re: [PATCH][Hashtable 0/6] Code review To: Jonathan Wakely Cc: "libstdc++@gcc.gnu.org" , gcc-patches References: <4a9df89d-d763-6e9e-e9d0-6e998152f2ec@gmail.com> <20200717101140.GA2827962@redhat.com> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: <76cc90a6-2bb6-e882-09df-9483a5eecce5@gmail.com> Date: Wed, 29 Jul 2020 11:35:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <20200717101140.GA2827962@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: fr X-Spam-Status: No, score=-9.2 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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: Wed, 29 Jul 2020 09:35:45 -0000 On 17/07/20 12:11 pm, Jonathan Wakely wrote: > N.B. the 0/6 entry of a patch series is supposed to be a cover letter > describing the changes in the series, not one of the patches. > > If you have patches 0/6, 1/6, 2/6 ... 6/6 then you actually have seven > patches, not six! > > Anyway ... > > On 19/12/19 20:17 +0100, François Dumont wrote: >> diff --git a/libstdc++-v3/include/bits/hashtable_policy.h >> b/libstdc++-v3/include/bits/hashtable_policy.h >> index f5809c7443a..b9320506bb2 100644 >> --- a/libstdc++-v3/include/bits/hashtable_policy.h >> +++ b/libstdc++-v3/include/bits/hashtable_policy.h >> @@ -172,6 +172,14 @@ namespace __detail >> >>   // Auxiliary types used for all instantiations of _Hashtable nodes >>   // and iterators. >> +  using __unique_keys_t = true_type; >> +  using __multi_keys_t = false_type; >> + >> +  using __constant_iterators_t = true_type; >> +  using __mutable_iterators_t = false_type; >> + >> +  using __hash_cached_t = true_type; >> +  using __hash_not_cached_t = false_type; > > This is an ABI change, and the benefit doesn't seem large enough to > justify it. It will result in code size increases for anything that > links objects compiled before and after the change. > > If we wanted to do this, I think it would be better to use enums, so: > > enum _Unique_keys { __unique_keys, __multi_keys }; > > Otherwise you can use __hash_cached_t where __unique_keys_t is meant > to be used, so all you've done is introduce more names for the same > things. > > If you want to replace the 'true' and 'false' literals with something > more descriptive, can't you just use constants? > > constexpr bool __hash_cached = true; > constexpr bool __hash_not_cached = false; > > Or just use comments: > >     struct _Local_iterator_base<_Key, _Value, _ExtractKey, >                 _H1, _H2, _Hash, /*cached=*/ false> > > Ok, I'll review this patch with this simpler approach. I even started to add comments in the patch you already approved.