From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 68CC73844046 for ; Tue, 28 Jul 2020 13:41:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 68CC73844046 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 [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 061D71E794; Tue, 28 Jul 2020 09:41:05 -0400 (EDT) Subject: Re: [PATCH] Change management of tdesc_arch_data To: Tom Tromey , gdb-patches@sourceware.org References: <20200727193605.2224169-1-tromey@adacore.com> From: Simon Marchi Message-ID: Date: Tue, 28 Jul 2020 09:41:00 -0400 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: <20200727193605.2224169-1-tromey@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, 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: 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: , X-List-Received-Date: Tue, 28 Jul 2020 13:41:07 -0000 On 2020-07-27 3:36 p.m., Tom Tromey wrote: > While working on something else, I noticed that tdesc_data_cleanup > took a void* parameter. Looking more into this, I found that > tdesc_use_registers expected a transfer of ownership. > > I think it's better to express this sort of thing via the type system, > when possible. This patch changes tdesc_data_alloc to return a unique > pointer, changes tdesc_use_registers to accept an rvalue reference, > and then adapts all the users. > > Note that a deleter structure is introduced to avoid having to move > tdesc_arch_data to the header file. Good idea. > @@ -784,10 +784,8 @@ tdesc_data_alloc (void) > architecture). */ > > void > -tdesc_data_cleanup (void *data_untyped) > +tdesc_arch_data_deleter::operator() (struct tdesc_arch_data *data) const I think the comment above this is not relevant, and should be made into the usual /* See foo.h. */. > @@ -1097,7 +1095,7 @@ set_tdesc_pseudo_register_reggroup_p > void > tdesc_use_registers (struct gdbarch *gdbarch, > const struct target_desc *target_desc, > - struct tdesc_arch_data *early_data, > + tdesc_arch_data_up &&early_data, > tdesc_unknown_register_ftype unk_reg_cb) > { > int num_regs = gdbarch_num_regs (gdbarch); > @@ -1112,7 +1110,6 @@ tdesc_use_registers (struct gdbarch *gdbarch, > > data = (struct tdesc_arch_data *) gdbarch_data (gdbarch, tdesc_data); > data->arch_regs = early_data->arch_regs; Not really important, but this could be an std::move to avoid copying the vector. And since the function now takes an rvalue reference, we know it's fine to do this. > diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c > index 57945d21db7..b78db89b4e2 100644 > --- a/gdb/tic6x-tdep.c > +++ b/gdb/tic6x-tdep.c > @@ -1140,7 +1140,7 @@ tic6x_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > { > struct gdbarch *gdbarch; > struct gdbarch_tdep *tdep; > - struct tdesc_arch_data *tdesc_data = NULL; > + tdesc_arch_data_up tdesc_data; Move this where this is used, in the if? That will avoid unnecessary construction/destruction. That would apply to other arches as well. Simon