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 B7318385802D for ; Wed, 14 Jul 2021 03:39:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B7318385802D Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-447-hHzz4B6eNsaIO0b-VYO7VQ-1; Tue, 13 Jul 2021 23:39:38 -0400 X-MC-Unique: hHzz4B6eNsaIO0b-VYO7VQ-1 Received: by mail-qt1-f200.google.com with SMTP id 12-20020ac8570c0000b02902520e309f5dso1096251qtw.8 for ; Tue, 13 Jul 2021 20:39:38 -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=412opCFbb2TEU89UetIJ7YwaFfYU5vyug2ltDpqUVDw=; b=MY+RJR3GKbmfXcU4134x9qYF+jWyHdR8DWySlKcruV1M11UZtKafMjgrKCStBRFQdF asIMoywJYsvtCuj3d9wHeskSm/Jftz3nHIoD8DlXUQJq3OU2l2xLyUsSAfx7K77ZWSKw B7yY/CPRJc2rX2cH46dsh9v8FgCe6+3JiLZPtYMfIYMgdm7bIl0pRRKYEK3evP7pkma3 SoEArqdazL1xs7lPAKTABeEdm/208q7/LByffs6EyDGZUY0OtEwZbqSXPJhl3fOzeTU/ Y0yLhN9oBuXPouv0R6+B2QN/fY9uSHkwmj+8C5YyUAVocLFRYAWmWGiRhSeCXTr4Oa6s UvKQ== X-Gm-Message-State: AOAM532qzAW0lpe6mZ6a8O42dZpyMLTkgUIoEYWTiBnxaMozSRerzYfA qwyErjNkF0FZkUAJTCn2eRtkYasHzDj0ImB8Bbk4Cu1Z8g1VTwWW3Hav5y3QTz5v38MuTjcmhLk t6WIvYZ2uoNxAYcXZaJG8bC1aszGy9Uys3iPIBethqn/7E3ukdfX6TL7xCcbpgH2dxg== X-Received: by 2002:ae9:e416:: with SMTP id q22mr7556324qkc.354.1626233977857; Tue, 13 Jul 2021 20:39:37 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzt6sXJ8uT1u62N7inqq3jEUR5/0lp4yYKX5WNx85LcxbGDtZW4hSVVYLGfL55ypbUtPSySPw== X-Received: by 2002:ae9:e416:: with SMTP id q22mr7556306qkc.354.1626233977530; Tue, 13 Jul 2021 20:39:37 -0700 (PDT) Received: from [192.168.1.148] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id d24sm458428qkk.61.2021.07.13.20.39.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 13 Jul 2021 20:39:36 -0700 (PDT) Subject: Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Martin Sebor , Jonathan Wakely , Richard Biener Cc: gcc-patches References: <4d503394-4e82-1d36-41ca-34315042775b@redhat.com> <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> From: Jason Merrill Message-ID: <2c60a7d0-3f60-b72f-c0f2-6fc7a4900740@redhat.com> Date: Tue, 13 Jul 2021 23:39:35 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <7ebb37f3-76c3-8e00-7852-c93bf142043a@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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 03:39:41 -0000 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. > 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. >> 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