From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id EvPQMhh/pV/oFgAAWB0awg (envelope-from ) for ; Fri, 06 Nov 2020 11:51:36 -0500 Received: by simark.ca (Postfix, from userid 112) id C3BFA1F08B; Fri, 6 Nov 2020 11:51:36 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 0A2921E58F for ; Fri, 6 Nov 2020 11:51:36 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 92A613857801; Fri, 6 Nov 2020 16:51:35 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 5B3A13851C03 for ; Fri, 6 Nov 2020 16:51:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5B3A13851C03 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E49F11E58F; Fri, 6 Nov 2020 11:51:31 -0500 (EST) Subject: Re: [PATCH] Split macro_buffer in two classes, fix Clang build To: Pedro Alves , gdb-patches@sourceware.org References: <20201106162055.32657-1-pedro@palves.net> From: Simon Marchi Message-ID: <59491b13-8210-6d20-d5b9-58a278fa3111@simark.ca> Date: Fri, 6 Nov 2020 11:51:31 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201106162055.32657-1-pedro@palves.net> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-11-06 11:20 a.m., Pedro Alves wrote: > GDB currently fails to build with (at least) Clang 10 and 11, due to: > > $ make > CXX macroexp.o > ../../src/gdb/macroexp.c:125:3: error: definition of implicit copy constructor for 'macro_buffer' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated-copy-dtor] > ~macro_buffer () > ^ > > Now, we could just add the copy constructor, like we already have a > copy assignment operator. And like that assignment operator, we would > assert that only shared buffers can be copied from. > > However, it is hard to see why only shared buffers need to be copied. > I mean, it must be true, otherwise macro support would be broken, > since currently GDB is relying on the default implementation of the > copy constructor, which just copies the fields, which can't work > correctly for the non-shared version. Still, it's not easy to tell > from the code that that is indeed correct, that there isn't some > corner case that would require copying a non-shared buffer. > > Or to put it simply - the tangling of shared and non-shared buffers in > the same macro_buffer struct makes this structure hard to understand. > > My reaction was -- try splitting the macro_buffer class into two > classes, one for non-shared buffers, and another for shared buffers. > > Comments and asserts like these: > > ... > SRC must be a shared buffer; DEST must not be one. */ > > static void > scan (struct macro_buffer *dest, > struct macro_buffer *src, > struct macro_name_list *no_loop, > const macro_scope &scope) > { > gdb_assert (src->shared); > gdb_assert (! dest->shared); > > ... made me suspect it should be possible. Then after the split it > should be easier to reimplement either of the classes if we want. > > So I decided to try splitting the struct in two distinct types, and > see where that leads. It turns out that there is really no good > reason for a single struct, no code that wants to work with either > shared or non-shared buffers. It's always shared for input being > parsed, and non-shared for output. > > This commit is the result. I named the new classes > shared_macro_buffer and growable_macro_buffer. > > A future direction could be for example to make shared_macro_buffer > wrap a string_view and growable_macro_buffer a std::string. With that > in mind, other than text/len, only the 'last_token' field is common to > both classes. I didn't feel like creating a base class just for that > single field. I think at least making growable_macro_buffer::text a gdb::unique_xmalloc_ptr would be good to do now (maybe as a subsequent patch), as it would let us delete the destructor and the explicit DISABLE_COPY_AND_ASSIGN, as well as simplify the release method. > > I constified shared_macro_buffer's 'text' field, which of course had > some knock-on effects, fixed in the patch. > > On the original warning issued by Clang -- now it is clear that really > only the shared version needs copy ctor/assign, so I added a copy > ctor, implemented on top of the assignment operator: > > shared_macro_buffer (const shared_macro_buffer &src) > { > *this = src; > } Are the copy constructor and assignment operators needed at all? It seems like we only do trivial copies of all fields. > @@ -106,45 +71,81 @@ struct macro_buffer > shared substring. */ > void set_shared (const char *addr, int len_) > { > - text = (char *) addr; > + text = addr; > len = len_; > - size = 0; > - shared = true; > } I'm wondering (for a later patch) if we could get rid of the set_shared method and just have one constructor (the one that has parameters). If so, it would make it clear that once created, the shared_macro_buffer objects never change (if that's indeed the case). And that they can never be "NULL". Anyway, thanks for doing this, it's been bugging me for a while. Simon