From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x830.google.com (mail-qt1-x830.google.com [IPv6:2607:f8b0:4864:20::830]) by sourceware.org (Postfix) with ESMTPS id 452003985472 for ; Wed, 14 Jul 2021 14:46:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 452003985472 Received: by mail-qt1-x830.google.com with SMTP id k3so2017683qtq.7 for ; Wed, 14 Jul 2021 07:46:19 -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-language :content-transfer-encoding; bh=xNQZzsfblNoObsGEMLcnpN2TbiULJIGP0xR3jWLtV/w=; b=mF3fj/CKtPTVJo2WVIWL9W4Da4eb6HwpAags6VDTY9wDKhcXFIwjQofx6LCKfaoRfS 8NCYnEVDRBHCG8GQSgHFJ4vlOgMvUM/sXpkzwyT64ukOGF5Dz6mfYejqGcYpM6WXXB+T iDjaFz3lc4Jwy0j/9DiazAwGoTWZg3FhexA+Vm3bH2tpP5B0alPPzR/AoIbLjxN7s/Zv Kio2OcSNi0kP9fwclxgl3pzhFVGc2KWhF7YbjKj0068hv3SANiyRWbW53JiLiIoJkn+z w7KfmZGVJ9rugYGK17HDJ+iNyUVLuejNcb7On/LYWsqX+UXyQjSUOEpF8HMuJHnzD37V qm3g== X-Gm-Message-State: AOAM530H5oV8M5wn7LuDle9EqInkB+Xzieh5XOARbr6poz4h1iADzWuS kVoQRr2HbNsLV/EKDZRYnGGrJezvwmg= X-Google-Smtp-Source: ABdhPJxbY9JzzM981kDPSn7aU60V29BE1J02glEe1+m2fyWHLEuYPswpEg5IC+HIqBsgqLQUHF8QCQ== X-Received: by 2002:ac8:7ee6:: with SMTP id r6mr9700976qtc.199.1626273978544; Wed, 14 Jul 2021 07:46:18 -0700 (PDT) Received: from [192.168.0.41] (75-166-102-22.hlrn.qwest.net. [75.166.102.22]) by smtp.gmail.com with ESMTPSA id x21sm1139988qkn.0.2021.07.14.07.46.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 14 Jul 2021 07:46:18 -0700 (PDT) Subject: Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Jason Merrill , Jonathan Wakely , Richard Biener Cc: gcc-patches References: <49569f1d-9856-55c7-b9e9-578bbd7c7b7a@gmail.com> <390c6652-0a1f-e8c4-d70d-56ced2f7b0fb@gmail.com> <0ee2488c-04a1-df3b-dd4f-92eec51a4ab2@redhat.com> <7ebb37f3-76c3-8e00-7852-c93bf142043a@gmail.com> <2c60a7d0-3f60-b72f-c0f2-6fc7a4900740@redhat.com> From: Martin Sebor Message-ID: <072a4715-2248-d836-948d-50426160de47@gmail.com> Date: Wed, 14 Jul 2021 08:46:17 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <2c60a7d0-3f60-b72f-c0f2-6fc7a4900740@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 14 Jul 2021 14:46:20 -0000 On 7/13/21 9:39 PM, Jason Merrill wrote: > On 7/13/21 4:02 PM, Martin Sebor wrote: >> On 7/13/21 12:37 PM, Jason Merrill wrote: >>> On 7/13/21 10:08 AM, Jonathan Wakely wrote: >>>> On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote: >>>>> Somebody with more C++ knowledge than me needs to approve the >>>>> vec.h changes - I don't feel competent to assess all effects of the >>>>> change. >>>> >>>> They look OK to me except for: >>>> >>>> -extern vnull vNULL; >>>> +static constexpr vnull vNULL{ }; >>>> >>>> Making vNULL have static linkage can make it an ODR violation to use >>>> vNULL in templates and inline functions, because different >>>> instantiations will refer to a different "vNULL" in each translation >>>> unit. >>> >>> The ODR says this is OK because it's a literal constant with the same >>> value (6.2/12.2.1). >>> >>> But it would be better without the explicit 'static'; then in C++17 >>> it's implicitly inline instead of static. >> >> I'll remove the static. >> >>> >>> But then, do we really want to keep vNULL at all?  It's a weird >>> blurring of the object/pointer boundary that is also dependent on vec >>> being a thin wrapper around a pointer.  In almost all cases it can be >>> replaced with {}; one exception is == comparison, where it seems to >>> be testing that the embedded pointer is null, which is a weird thing >>> to want to test. >> >> The one use case I know of for vNULL where I can't think of >> an equally good substitute is in passing a vec as an argument by >> value.  The only way to do that that I can think of is to name >> the full vec type (i.e., the specialization) which is more typing >> and less generic than vNULL.  I don't use vNULL myself so I wouldn't >> miss this trick if it were to be removed but others might feel >> differently. > > In C++11, it can be replaced by {} in that context as well. Cool. I thought I'd tried { } here but I guess not. > >> If not, I'm all for getting rid of vNULL but with over 350 uses >> of it left, unless there's some clever trick to make the removal >> (mostly) effortless and seamless, I'd much rather do it independently >> of this initial change. I also don't know if I can commit to making >> all this cleanup. > > I already have a patch to replace all but one use of vNULL, but I'll > hold off with it until after your patch. So what's the next step? The patch only removes a few uses of vNULL but doesn't add any. Is it good to go as is (without the static and with the additional const changes Richard suggested)? This patch is attached to my reply to Richard: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html Martin > >>> Somewhat relatedly, use of vec variables or fields seems almost >>> always a mistake, as they need explicit .release() that could be >>> automatic with auto_vec, and is easy to forget.  For instance, the >>> recursive call in get_all_loop_exits returns a vec that is never >>> released.  And I see a couple of leaks in the C++ front end as well. >> >> I agree.  The challenge I ran into with changing vec fields is with >> code that uses the vec member as a reference to auto_vec.  This is >> the case in gcc/ipa-prop.h, for instance.  Those instances could >> be changed to auto_vec references or pointers but again it's a more >> intrusive change than the simple replacements I was planning to make >> in this first iteration. >> >> So in summary, I agree with the changes you suggest.  Given their >> scope I'd prefer not to make them in the same patch, and rather make >> them at some point in the future when I or someone else has the time >> and energy.  I'm running out. > > Oh, absolutely. > > Jason >