From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 3267D3861975 for ; Thu, 6 Aug 2020 10:23:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3267D3861975 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-502-GTNOn0gjMLeNG0BCA3dI8Q-1; Thu, 06 Aug 2020 06:23:11 -0400 X-MC-Unique: GTNOn0gjMLeNG0BCA3dI8Q-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 691D4101C8A5; Thu, 6 Aug 2020 10:23:10 +0000 (UTC) Received: from localhost (unknown [10.33.36.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id D9E627B93B; Thu, 6 Aug 2020 10:23:09 +0000 (UTC) Date: Thu, 6 Aug 2020 11:23:08 +0100 From: Jonathan Wakely To: Richard Biener Cc: Andrew MacLeod , GCC Patches , Martin Jambor , Aldy Hernandez , Jeff Law Subject: Re: std:vec for classes with constructor? (Was: Re: [patch] multi-range implementation for value_range (irange)) Message-ID: <20200806102308.GV3400@redhat.com> References: <5EE6522C-086B-42E0-B42A-F291E09C422A@gmail.com> <83ac1879-ce77-004c-3934-dddd7b9e22c6@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, 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: Thu, 06 Aug 2020 10:23:14 -0000 On 06/08/20 08:57 +0200, Richard Biener wrote: >On Thu, Aug 6, 2020 at 3:07 AM Andrew MacLeod wrote: >> >> On 8/5/20 12:54 PM, Richard Biener via Gcc-patches wrote: >> > On August 5, 2020 5:09:19 PM GMT+02:00, Martin Jambor wrote: >> >> On Fri, Jul 31 2020, Aldy Hernandez via Gcc-patches wrote: >> >> [...] >> >> >> >>> * ipa-cp changes from vec to std::vec. >> >>> >> >>> We are using std::vec to ensure constructors are run, which they >> >> aren't >> >>> in our internal vec<> implementation. Although we usually steer away >> >>> from using std::vec because of interactions with our GC system, >> >>> ipcp_param_lattices is only live within the pass and allocated with >> >> calloc. >> >> Ummm... I did not object but I will save the URL of this message in the >> >> archive so that I can waive it in front of anyone complaining why I >> >> don't use our internal vec's in IPA data structures. >> >> >> >> But it actually raises a broader question: was this supposed to be an >> >> exception, allowed only not to complicate the irange patch further, or >> >> will this be generally accepted thing to do when someone wants to have >> >> a >> >> vector of constructed items? >> > It's definitely not what we want. You have to find another solution to this problem. >> > >> > Richard. >> > >> >> Why isn't it what we want? >> >> This is a small vector local to the pass so it doesn't interfere with >> our PITA GTY. >> The class is pretty straightforward, but we do need a constructor to >> initialize the pointer and the max-size field. There is no allocation >> done per element, so a small number of elements have a couple of fields >> initialized per element. We'd have to loop to do that anyway. >> >> GCC's vec<> does not provide he ability to run a constructor, std::vec >> does. > >Other places in the compiler use placement new here. The only >complication is re-allocation which would require move CTORs which >up to a few weeks ago were not available. But of course we want >our own re-allocation policy, thus vec<>, not std::vector. > >Also you do > >#include > >inside ipa-fnsummary.c - you should know better to not include >system headers from .c files - they need to go in system.h. >You should do a > >#define INCLUDE_VECTOR > >before the system.h include in ipa-fnsummary.c instead. > >I find it only mildly amusing that review & approval of this happened >behind the scenes ... > >I scanned the code but didn't spot the point where you'd need to run >CTORs? Can you point me to that? > >> I quizzed some libstdc++ folks, and there has been a lot of >> optimizations done on std::vec over the last few years,.. They think its >> pretty good now, and we were encouraged to use it. >> >> We can visit the question tho... What is the rationale for not using >> std::vec in the compiler? We currently use std::swap, std:pair, >> std::map, std::sort, and a few others. > >We're using std::swap and std::pair throughoug because those are lean. >One of the main motivations to not use the STL is because of TU >explosion when including lots of those large headers. Bootstrap times >are still an issue. > >There's also code size issues when we have both std::vector >and vec instantiations. And as you say vec<> isn't going away >because there's no way to make std::vector & friends GC aware. > >Then there's consistency across the code-base. A big mess will >scare of new contributors. So do weird types that don't run constructors and require manually constructing things into buffers with placement new, or only using types with trivial default construcrors. It's not 1994. Containers should own and manage their contents. Maybe adding support for non-trivial construction to vec and making it movable (and not copyable) would make it more useful. >Oh, and existing uses are mostly mistakes and are _not_ a reason >to add more "exceptions" - instead they are a reason to rectify those >mistakes. > >Richard. > >> is there some aspect of using std::vec I am not aware of that makes it >> something we need to avoid? >> >> Andrew >> >> >> >> >> >