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 58E653857831 for ; Tue, 1 Jun 2021 21:38:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 58E653857831 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-275-YZT-Ym1KNyGYxXr-KatIkQ-1; Tue, 01 Jun 2021 17:38:45 -0400 X-MC-Unique: YZT-Ym1KNyGYxXr-KatIkQ-1 Received: by mail-qv1-f71.google.com with SMTP id l16-20020ad442500000b029021344b553c5so274286qvq.5 for ; Tue, 01 Jun 2021 14:38:45 -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=MiTLlJOP9C9drKuXb0T1zW4rBzFUUjzBEse5mtloDkM=; b=ibAdsQPWpchJbvJ3zjpipv64mPVD2P5jkNhQ9lr+xBnIQZ18mq4TK7VOpV0JEyFELI GQIfkptvluvg1j1RWHU+oIuVtTd7tbjL3GVMggxdhxvfUN+c+Lms2izGG7hs+GtoU1UK nDDD9IzaJyZYFlow+gyeP84rIBf5k/jEW4KEfdTWT0gc0yLAgZ56QCqf8XqSi18XXMs5 YFtXL3XiQppWjJ2v3F+A4cxFrrLugbr864AAMJ0558eKwKgksnwqwDV3P4nYoPgYFLSk 79ivKijVUKXs2I/pO8iXaGgsFw+AAqBGEHEzCvP5ljb+y2ULm6edz+T2BCo8U4kVMJtH sCDg== X-Gm-Message-State: AOAM532sKrvCZE1d9aLrBmMVJXB8BaIAD6pLQxAVtwpDk8aoy6+Y3Mi0 KsiT49RS2H87LvC448QCF4YLzz272NtZ8CV9OAv2G6g8yID1ugPmXQ7C1DVYxE68s0D0CTC9JmM ZAwtAdHVKko0GTpc1rg== X-Received: by 2002:a37:6249:: with SMTP id w70mr24289917qkb.27.1622583524846; Tue, 01 Jun 2021 14:38:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyNPU742gvQEsgzKq7486IdoqiTfAxGQWWgBPvXps3HCIJjkrmFqfoxCSdbNSPkRoxz3RSjlg== X-Received: by 2002:a37:6249:: with SMTP id w70mr24289894qkb.27.1622583524550; Tue, 01 Jun 2021 14:38:44 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id b189sm1780507qkc.91.2021.06.01.14.38.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Jun 2021 14:38:44 -0700 (PDT) Subject: Re: [PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Martin Sebor , Richard Biener Cc: gcc-patches , Jonathan Wakely References: <91545a73-12af-33b2-c6e7-119b5a21de60@gmail.com> <4d503394-4e82-1d36-41ca-34315042775b@redhat.com> From: Jason Merrill Message-ID: Date: Tue, 1 Jun 2021 17:38:43 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: 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_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: Tue, 01 Jun 2021 21:38:49 -0000 On 6/1/21 3:56 PM, Martin Sebor wrote: > On 5/27/21 2:53 PM, Jason Merrill wrote: >> On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote: >>> On 4/27/21 8:04 AM, Richard Biener wrote: >>>> On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor wrote: >>>>> >>>>> On 4/27/21 1:58 AM, Richard Biener wrote: >>>>>> On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches >>>>>> wrote: >>>>>>> >>>>>>> PR 90904 notes that auto_vec is unsafe to copy and assign because >>>>>>> the class manages its own memory but doesn't define (or delete) >>>>>>> either special function.  Since I first ran into the problem, >>>>>>> auto_vec has grown a move ctor and move assignment from >>>>>>> a dynamically-allocated vec but still no copy ctor or copy >>>>>>> assignment operator. >>>>>>> >>>>>>> The attached patch adds the two special functions to auto_vec along >>>>>>> with a few simple tests.  It makes auto_vec safe to use in >>>>>>> containers >>>>>>> that expect copyable and assignable element types and passes >>>>>>> bootstrap >>>>>>> and regression testing on x86_64-linux. >>>>>> >>>>>> The question is whether we want such uses to appear since those >>>>>> can be quite inefficient?  Thus the option is to delete those >>>>>> operators? >>>>> >>>>> I would strongly prefer the generic vector class to have the >>>>> properties >>>>> expected of any other generic container: copyable and assignable.  If >>>>> we also want another vector type with this restriction I suggest to >>>>> add >>>>> another "noncopyable" type and make that property explicit in its >>>>> name. >>>>> I can submit one in a followup patch if you think we need one. >>>> >>>> I'm not sure (and not strictly against the copy and assign). >>>> Looking around >>>> I see that vec<> does not do deep copying.  Making auto_vec<> do it >>>> might be surprising (I added the move capability to match how vec<> >>>> is used - as "reference" to a vector) >>> >>> The vec base classes are special: they have no ctors at all (because >>> of their use in unions).  That's something we might have to live with >>> but it's not a model to follow in ordinary containers. >> >> I don't think we have to live with it anymore, now that we're writing >> C++11. >> >>> The auto_vec class was introduced to fill the need for a conventional >>> sequence container with a ctor and dtor.  The missing copy ctor and >>> assignment operators were an oversight, not a deliberate feature. >>> This change fixes that oversight. >>> >>> The revised patch also adds a copy ctor/assignment to the auto_vec >>> primary template (that's also missing it).  In addition, it adds >>> a new class called auto_vec_ncopy that disables copying and >>> assignment as you prefer. >> >> Hmm, adding another class doesn't really help with the confusion richi >> mentions.  And many uses of auto_vec will pass them as vec, which will >> still do a shallow copy.  I think it's probably better to disable the >> copy special members for auto_vec until we fix vec<>. > > There are at least a couple of problems that get in the way of fixing > all of vec to act like a well-behaved C++ container: > > 1) The embedded vec has a trailing "flexible" array member with its > instances having different size.  They're initialized by memset and > copied by memcpy.  The class can't have copy ctors or assignments > but it should disable/delete them instead. > > 2) The heap-based vec is used throughout GCC with the assumption of > shallow copy semantics (not just as function arguments but also as > members of other such POD classes).  This can be changed by providing > copy and move ctors and assignment operators for it, and also for > some of the classes in which it's a member and that are used with > the same assumption. > > 3) The heap-based vec::block_remove() assumes its elements are PODs. > That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862 > and tree-vect-patterns.c). > > I took a stab at both and while (1) is easy, (2) is shaping up to > be a big and tricky project.  Tricky because it involves using > std::move in places where what's moved is subsequently still used. > I can keep plugging away at it but it won't change the fact that > the embedded and heap-based vecs have different requirements. > > It doesn't seem to me that having a safely copyable auto_vec needs > to be put on hold until the rats nest above is untangled.  It won't > make anything worse than it is.  (I have a project that depends on > a sane auto_vec working). > > A couple of alternatives to solving this are to use std::vector or > write an equivalent vector class just for GCC. It occurs to me that another way to work around the issue of passing an auto_vec by value as a vec, and thus doing a shallow copy, would be to add a vec ctor taking an auto_vec, and delete that. This would mean if you want to pass an auto_vec to a vec interface, it needs to be by reference. We might as well do the same for operator=, though that isn't as important. Jason