From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x735.google.com (mail-qk1-x735.google.com [IPv6:2607:f8b0:4864:20::735]) by sourceware.org (Postfix) with ESMTPS id 3F548388A035 for ; Tue, 27 Apr 2021 15:52:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3F548388A035 Received: by mail-qk1-x735.google.com with SMTP id 190so3749563qkl.11 for ; Tue, 27 Apr 2021 08:52:57 -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; bh=A3a0zWVRMiq+tiDL/3RNkoXj4sbopkytg3u8HN6lkDg=; b=qwJtRCY0h67crhdKml5k97SFymQD5m44kOHgynUNFcnolZmeTDQHjymTSVWQGr/sQw 3gP1FIOokHiFl9By+kONSQ9+/Ijm6nwEN6jdMDyeXVrLg3iW8ouFDHkX8nHswT2XQr2A ssekMXixe0oZyfdPimu53rJiUxQvtY2tKGONcRilQYIOWa6EffIEzUt8o2NjpNta7GZk 9UOsGi18CyLO2EQW24FWzI34FkzbxlA3d5IFA0coxbpQeEdcibT1ZVrPXUxhrtws4G/6 Utny8Nr/KADQjo5LwvSJT6AvW3sevzY7D9jCubYQ3bWJrg4e/5dLcnT8ZkXAfX15Bv8q 9A2A== X-Gm-Message-State: AOAM5301TOiNp0GF5M7IixIQPeW7Q7aYZMHSqcLZecQvLO8UFhBxsWgD 7hzeXQ9FkWKzYKrejrdLofDbnthebxA= X-Google-Smtp-Source: ABdhPJxfroNtSS9EV10KSmdwioLik3E9MsxbRSVnsRL49p+8h3efz9+nCOUddS6NSGsjvgvh/ysOew== X-Received: by 2002:a05:620a:8dd:: with SMTP id z29mr15832945qkz.492.1619538776603; Tue, 27 Apr 2021 08:52:56 -0700 (PDT) Received: from [192.168.0.41] (71-218-14-121.hlrn.qwest.net. [71.218.14.121]) by smtp.gmail.com with ESMTPSA id i6sm3075997qkf.96.2021.04.27.08.52.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Apr 2021 08:52:56 -0700 (PDT) Subject: Re: [PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Richard Biener Cc: gcc-patches References: From: Martin Sebor Message-ID: <91545a73-12af-33b2-c6e7-119b5a21de60@gmail.com> Date: Tue, 27 Apr 2021 09:52:54 -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: multipart/mixed; boundary="------------EDCF7DFA700F56AEB0483005" Content-Language: en-US X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, 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: Tue, 27 Apr 2021 15:52:59 -0000 This is a multi-part message in MIME format. --------------EDCF7DFA700F56AEB0483005 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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. 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. It also disables copying for the auto_string_vec class. Martin --------------EDCF7DFA700F56AEB0483005 Content-Type: text/x-patch; charset=UTF-8; name="gcc-90904.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-90904.diff" PR middle-end/90904 - vec assignment and copying undefined gcc/ChangeLog: PR middle-end/90904 * vec.c (test_copy_assign): New function. (vec_c_tests): Call it. * vec.h (vec_assign): New function. (auto_vec copy ctor): Define. (auto_vec::operator=): Define. (auto_vec_no_copy): New class template. (auto_string_vec): Disable copying/assignment. diff --git a/gcc/vec.c b/gcc/vec.c index f9dbb2cac31..f15530d1e43 100644 --- a/gcc/vec.c +++ b/gcc/vec.c @@ -317,6 +317,86 @@ test_safe_push () ASSERT_EQ (7, v[2]); } + +/* Verify that auto_vec copy ctor and assignment work correctly. */ + +template +void test_copy_assign () +{ + typedef auto_vec test_vec; + + test_vec a0; + test_vec b0 (0); + test_vec c0 (7); + ASSERT_EQ (0, b0.length ()); + ASSERT_EQ (0, c0.length ()); + + a0 = a0; + ASSERT_EQ (0, a0.length ()); + b0 = a0; + ASSERT_EQ (0, b0.length ()); + c0 = a0; + ASSERT_EQ (0, c0.length ()); + + test_vec a3; + a3.safe_push (5); + a3.safe_push (6); + a3.safe_push (7); + + test_vec b3 (a3); + ASSERT_EQ (3, b3.length ()); + ASSERT_EQ (5, b3[0]); + ASSERT_EQ (6, b3[1]); + ASSERT_EQ (7, b3[2]); + + test_vec c3; + c3 = b3; + ASSERT_EQ (3, c3.length ()); + ASSERT_EQ (5, c3[0]); + ASSERT_EQ (6, c3[1]); + ASSERT_EQ (7, c3[2]); + + test_vec d4; + d4.safe_push (1); + d4.safe_push (2); + d4.safe_push (3); + d4.safe_push (4); + + c3 = d4; + ASSERT_EQ (4, c3.length ()); + ASSERT_EQ (1, c3[0]); + ASSERT_EQ (2, c3[1]); + ASSERT_EQ (3, c3[2]); + ASSERT_EQ (4, c3[3]); + + d4 = a3; + ASSERT_EQ (3, d4.length ()); + ASSERT_EQ (5, d4[0]); + ASSERT_EQ (6, d4[1]); + ASSERT_EQ (7, d4[2]); + + a3 = b0; + ASSERT_EQ (0, a3.length ()); + + b3 = b0; + ASSERT_EQ (0, b3.length ()); + + c3 = c0; + ASSERT_EQ (0, c3.length ()); + + b0 = d4; + ASSERT_EQ (3, b0.length ()); + ASSERT_EQ (5, b0[0]); + ASSERT_EQ (6, b0[1]); + ASSERT_EQ (7, b0[2]); + + c0 = d4; + ASSERT_EQ (3, c0.length ()); + ASSERT_EQ (5, c0[0]); + ASSERT_EQ (6, c0[1]); + ASSERT_EQ (7, c0[2]); +} + /* Verify that vec::truncate works correctly. */ static void @@ -549,6 +629,8 @@ vec_c_tests () { test_quick_push (); test_safe_push (); + test_copy_assign<0> (); + test_copy_assign<2> (); test_truncate (); test_safe_grow_cleared (); test_pop (); diff --git a/gcc/vec.h b/gcc/vec.h index 24df2db0eeb..f5dd5bb329a 100644 --- a/gcc/vec.h +++ b/gcc/vec.h @@ -1509,7 +1509,41 @@ public: want to ask for internal storage for vectors on the stack because if the size of the vector is larger than the internal storage that space is wasted. */ + template +class auto_vec; + +/* Safely assign elements from SRC to DST, invoking the copy assignment + operator on the initial elements and the copy ctor on the excess. */ + +template +auto_vec& vec_assign (auto_vec &dst, const auto_vec &src) +{ + unsigned n0 = dst.length (); + const unsigned n1 = src.length (); + if (n0 < n1) + dst.safe_grow (n1); + else + { + dst.truncate (n1); + n0 = n1; + } + + T *pdst = dst.address (); + const T *psrc = src.address (); + + /* Copy-assign over the first N0 elements. */ + for (unsigned i = 0; i != n0; ++i) + pdst[i] = psrc[i]; + + if (n0 < n1) + /* Copy-construct elements in excess of N0. */ + vec_copy_construct (pdst + n0, psrc + n0, n1 - n0); + + return dst; +} + +template class auto_vec : public vec { public: @@ -1536,11 +1570,34 @@ public: this->release (); } + /* Copy ctor. */ + auto_vec (const auto_vec &); + + /* Copy asssignment. */ + auto_vec& operator= (const auto_vec &r) + { + return vec_assign (*this, r); + } + private: vec m_auto; T m_data[MAX (N - 1, 1)]; }; +/* Copy elements from R to *THIS. */ + +template +auto_vec::auto_vec (const auto_vec& r) +{ + const unsigned n = r.length (); + this->create (n); + if (n) + { + vec_copy_construct (this->address (), r.address (), n); + this->m_vec->m_vecpfx.m_num = n; + } +} + /* auto_vec is a sub class of vec whose storage is released when it is destroyed. */ template @@ -1551,12 +1608,24 @@ public: auto_vec (size_t n CXX_MEM_STAT_INFO) { this->create (n PASS_MEM_STAT); } ~auto_vec () { this->release (); } + /* Copy ctor. */ + auto_vec (const auto_vec&); + + /* Move ctor from vec that's not auto_vec. */ auto_vec (vec&& r) { gcc_assert (!r.using_auto_storage ()); this->m_vec = r.m_vec; r.m_vec = NULL; } + + /* Copy asssignment. */ + auto_vec& operator= (const auto_vec &rhs) + { + return vec_assign (*this, rhs); + } + + /* Move asssignment from vec that's not auto_vec. */ auto_vec& operator= (vec&& r) { gcc_assert (!r.using_auto_storage ()); @@ -1567,6 +1636,35 @@ public: } }; +/* Copy elements from R to *THIS. */ + +template +auto_vec::auto_vec (const auto_vec& r) +{ + const unsigned n = r.length (); + this->create (n); + if (n) + { + vec_copy_construct (this->address (), r.address (), n); + this->m_vec->m_vecpfx.m_num = n; + } +} + +/* Same as auto_vec but with deleted copy ctor and assignment + operator to detect uses of these special functions in contexts + where they may be inefficient. */ + +template +class auto_vec_no_copy: public auto_vec +{ +public: + /* Inherit all ctors. */ + using auto_vec::auto_vec; + +private: + /* Prevent copying and assignment. */ + DISABLE_COPY_AND_ASSIGN (auto_vec_no_copy); +}; /* Allocate heap memory for pointer V and create the internal vector with space for NELEMS elements. If NELEMS is 0, the internal @@ -1587,7 +1685,11 @@ vec_alloc (vec *&v, unsigned nelems CXX_MEM_STAT_INFO) class auto_string_vec : public auto_vec { public: + auto_string_vec (): auto_vec () { } ~auto_string_vec (); + +private: + DISABLE_COPY_AND_ASSIGN (auto_string_vec); }; /* A subclass of auto_vec that deletes all of its elements on --------------EDCF7DFA700F56AEB0483005--