From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x230.google.com (mail-oi1-x230.google.com [IPv6:2607:f8b0:4864:20::230]) by sourceware.org (Postfix) with ESMTPS id 81CE73898026 for ; Thu, 24 Jun 2021 15:01:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 81CE73898026 Received: by mail-oi1-x230.google.com with SMTP id w127so7606954oig.12 for ; Thu, 24 Jun 2021 08:01:58 -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=AbB23zsuvUxLc+xZqr25Yo9/3NCxy0bQRT3QbfriYH0=; b=AYkEjBTAWUnnbSUBLfVgq0l0hvkTcCPiGl1dez5CLmyMcBCkOZaGUtWb8nK8sWTBth VRSo1Jqxljvf+Ownai+j0PYggERndHvDw6Rz2dj07yR92/iUp706+TbCy0JdRDTTIwmG MeJTkv6QMv5+J4MzbLmdychFE0hOsvA//s0exEU880cONXsKkWVHy8B+yL5mRJPkvE9T RHcDFBLiPS7m6sUbTEieAcUyBXGz8fw4VVzXi5lrb1UawZK8Z5z0hlqs7/ksAlMsiQvl zYWd8ik/cpQ8QsjlpJW1Dtk4ichVgjyITclNo9L1jNt89uKc26+UxRyn7VzLSFye9prY a9cw== X-Gm-Message-State: AOAM532BaraiNjIl+sMhiOdi9sjHD9qFwBY1FUUDN98E5r7N4Lw2J6iW TsY5g/GmSFyzONRPjyDpEsE= X-Google-Smtp-Source: ABdhPJxNAFTvMlYZGWRRWpTSaBLEUjfGR4Ef9nsK+PytgHtrhE4TfMXxSbbURK7v5I8bdALFWpiGYQ== X-Received: by 2002:aca:5cc1:: with SMTP id q184mr7684569oib.135.1624546917240; Thu, 24 Jun 2021 08:01:57 -0700 (PDT) Received: from [192.168.0.41] (97-118-105-195.hlrn.qwest.net. [97.118.105.195]) by smtp.gmail.com with ESMTPSA id b22sm655653oov.31.2021.06.24.08.01.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 24 Jun 2021 08:01:56 -0700 (PDT) Subject: Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec To: Richard Biener Cc: Trevor Saunders , Richard Biener via Gcc-patches , Richard Sandiford References: <20210615055922.27205-5-tbsaunde@tbsaunde.org> <9a54c5e1-2d41-f499-e176-84913b810b67@gmail.com> <125764da-111e-7e44-b2ba-8389644423e8@gmail.com> <08396d8a-ff3e-ba6e-756a-ad4226b920a7@gmail.com> <8b6b78a2-31d1-f3a3-90a9-30673753a409@gmail.com> From: Martin Sebor Message-ID: Date: Thu, 24 Jun 2021 09:01:55 -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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, 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: Thu, 24 Jun 2021 15:02:00 -0000 On 6/24/21 3:27 AM, Richard Biener wrote: > On Thu, Jun 24, 2021 at 12:56 AM Martin Sebor wrote: >> >> On 6/23/21 1:43 AM, Richard Biener wrote: >>> On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders wrote: >>>> >>>> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: >>>>> On 6/21/21 1:15 AM, Richard Biener wrote: >>> [...] >>>>>> >>>>>> But maybe I'm misunderstanding C++ too much :/ >>>>>> >>>>>> Well, I guess b) from above means auto_vec<> passing to >>>>>> vec<> taking functions will need changes? >>>>> >>>>> Converting an auto_vec object to a vec slices off its data members. >>>>> The auto_vec specialization has no data members so that's not >>>>> a bug in and of itself, but auto_vec does have data members >>>>> so that would be a bug. The risk is not just passing it to >>>>> functions by value but also returning it. That risk was made >>>>> worse by the addition of the move ctor. >>>> >>>> I would agree that the conversion from auto_vec<> to vec<> is >>>> questionable, and should get some work at some point, perhaps just >>>> passingauto_vec references is good enough, or perhaps there is value in >>>> some const_vec view to avoid having to rely on optimizations, I'm not >>>> sure without looking more at the usage. >>> >>> We do need to be able to provide APIs that work with both auto_vec<> >>> and vec<>, I agree that those currently taking a vec<> by value are >>> fragile (and we've had bugs there before), but I'm not ready to say >>> that changing them all to [const] vec<>& is OK. The alternative >>> would be passing a const_vec<> by value, passing that along to >>> const vec<>& APIs should be valid then (I can see quite some API >>> boundary cleanups being necessary here ...). >>> >>> But with all this I don't know how to adjust auto_vec<> to no >>> longer "decay" to vec<> but still being able to pass it to vec<>& >>> and being able to call vec<> member functions w/o jumping through >>> hoops. Any hints on that? private inheritance achieves the first >>> but also hides all the API ... >> >> The simplest way to do that is by preventing the implicit conversion >> between the two types and adding a to_vec() member function to >> auto_vec as Jason suggested. This exposes the implicit conversions >> to the base vec, forcing us to review each and "fix" it either by >> changing the argument to either vec& or const vec&, or less often >> to avoid the indirection if necessary, by converting the auto_vec >> to vec explicitly by calling to_vec(). The attached diff shows >> a very rough POC of what that might look like. A side benefit >> is that it improves const-safety and makes the effects of functions >> on their callers easier to understand. >> >> But that's orthogonal to making auto_vec copy constructible and copy >> assignable. That change can be made independently and with much less >> effort and no risk. > > There's a bunch of stuff I can't review because of lack of C++ knowledge > (the vNULL changes). > > I suppose that > > + /* Prevent implicit conversion from auto_vec. Use auto_vec::to_vec() > + instead. */ > + template > + vec (auto_vec &) = delete; > + > + template > + void operator= (auto_vec &) = delete; > > still allows passing auto_vec<> to [const] vec<> & without the .to_vec so > it's just the by-value passing that becomes explicit? If so then I agree > this is an improvement. The patch is likely incomplete though or is > it really only so few signatures that need changing? Yes, to both questions. I just wanted to show how we might go about making this transition. I converted a few files but many more remain. I stopped when changing a vec argument to const vec& started to cause errors due to missing const down the line (e.g., assigning the address of a vec element to an uqualified pointer). I'll need to follow where that pointer flows and see if it's used to modify the object or if it too could be made const. To me that seems worthwhile doing now but it makes progress slow. A separate question is whether the vec value arguments to functions that modify them should be changed to references or pointers. Both kinds of APIs exist but the latter seems prevalent. Changing them to the latter means more churn. Martin