From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id CJQ1HzZUH2HeLQAAWB0awg (envelope-from ) for ; Fri, 20 Aug 2021 03:05:26 -0400 Received: by simark.ca (Postfix, from userid 112) id 7B2AD1EDFB; Fri, 20 Aug 2021 03:05:26 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.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 6B87B1EA7E for ; Fri, 20 Aug 2021 03:05:25 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F246B38930C1 for ; Fri, 20 Aug 2021 07:05:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F246B38930C1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1629443125; bh=Ti8218vwZFRIiGd4cLLlXgVrbA0+Kt49BjUxr6XHb+I=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=mFuUhgJ1RDmyt04dO0G5QVS7SgGMX0LIhZ6maGNeEsqnrSD6pSyfAqUSR9c7cajUD PfdPqI5f6BcJl8jg4li0ShSZuRvpfeCCRaOo41izYMexAnsQxL5aWxW1i6oiV3DTyO czCFzH4HCeUyVMU0gjjy35ikUGk4Elxt6r+GR2IU= Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 5D38D38930D9 for ; Fri, 20 Aug 2021 07:04:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5D38D38930D9 Received: from lsix-M11x-R2 (unknown [IPv6:2a01:e34:ef9a:d0b0:758b:c721:4e44:944f]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 1107E80D76; Fri, 20 Aug 2021 07:04:54 +0000 (UTC) Date: Fri, 20 Aug 2021 08:04:49 +0100 To: Simon Marchi Subject: Re: [PATCH v2 3/4] gdb: Have setter and getter callbacks for settings Message-ID: <20210820070449.5kfsm6xyl4qnfrnj@lsix-M11x-R2> References: <20210808192302.3768766-1-lsix@lancelotsix.com> <20210808192302.3768766-4-lsix@lancelotsix.com> <727ab80a-c72d-960a-6d6e-a731aaa147dd@polymtl.ca> <20210810221852.bqbb6opnrlaqgob5@Plymouth> <7de0a03d-77bf-3b09-9a11-4a109afce8a8@polymtl.ca> <20210811195946.ls3e2d2sbpt24o2e@ubuntu.lan> <435b5531-5f62-d28d-9cef-c5756e8f014e@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <435b5531-5f62-d28d-9cef-c5756e8f014e@polymtl.ca> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Fri, 20 Aug 2021 07:04:54 +0000 (UTC) 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: , From: Lancelot SIX via Gdb-patches Reply-To: Lancelot SIX Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On Thu, Aug 19, 2021 at 05:03:32PM -0400, Simon Marchi wrote: > On 2021-08-11 4:00 p.m., Lancelot SIX wrote: > >> Another option would be to use > >> > >> gdb::optional var; > >> > >> in cmd_list_element, and make it such that a setting can never exist in > >> an invalid state. Two constructor would exist, one to construct a > >> setting with a pointer to a buffer and another with the setter/getter. > >> There would be no need (I think) for set_p and set_accessors. > > > > This approach would be a problem within param_smob > > (gdb/guile/scm-param.c). This struct is memset initialized, so we cannot > > have one of its member which is not a PODType (see the redefinition of > > memset in gdbsupport/poison.h). We could have a pointer to a properly > > constructed setting within a param_smob plus some manual memory > > management, but I am not sure it is worth it. > > Tony Tye sent me some comments about this patch, which made me > reconsider the approach a bit. I don't see the problem with > Guile / param_smob. In your patch, setting_wrapper is just a transient > object, it exists just for a moment to make the glue between a setting > (which can be handled by the common command code) and a param_smob. But > it never needs to be stored anywhere (in a POD type or otherwise). > > I implemented this approach of using gdb::optional and constructors, and > it seems to work pretty well. I prefer it, because, as mentioned > before, the setting object can't exist in an invalid state. And that > is one less thing to worry about. Hi, I think that in my first iteration on this series I had a non trivial ctor and it triggered a problem around the guile code. I did not dig too much in this direction and did not reconsider later. If gdb::optional can be used, I also prefer the option where we only use non trivial constructors and ensure that the object can only exist in a valid state. > > The other comment was that the unions setting_getter and setting_setter > plus all the get_setting_setter / get_setting_getter implementations > were not really needed but add a lot of code. Tony suggested to save > the getter and setter function pointers to type-erased variables, just > like we do for m_var. And I don't think we lose any safety, as > var_type_uses already validates that we don't pass some invalid type T, > and that T matches var_type. That removes a lot of boilerplate. I > implemented this, and stored the getters / setters as `void (*) ()`. > Storing them as `void *` wouldn't work, because you can't freely convert > function pointers to data pointers and vice versa. > I did use a union there because I could not use 'void *'. I did not consider to cast to 'void (*) ()' but indeed, it will be a lot less code, more consistent with the type erasure done with m_var and as safe as the union option. Only good reasons to go for this approach. > I started from your users/lsix/refactor-typesafe-var, made changes to > the various patches, and uploaded it to > users/simark/refactor-typesafe-var. > I’ll have a look as soon as I can. Thanks for doing this! > Let me know what you think. > I like both of the changes you have suggested. They are improvements over my patch set. Thanks a lot to you and Tony for the reviews, suggestions and improvements. Lancelot. > Simon