From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id pu70KG/7EmGBDQAAWB0awg (envelope-from ) for ; Tue, 10 Aug 2021 18:19:27 -0400 Received: by simark.ca (Postfix, from userid 112) id 917AD1EDF7; Tue, 10 Aug 2021 18:19:27 -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=ham 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 A33791E813 for ; Tue, 10 Aug 2021 18:19:26 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E6F67385EC52 for ; Tue, 10 Aug 2021 22:19:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E6F67385EC52 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1628633965; bh=NTkiRxRDBShwsgtRtkqCGwdGCK00gJPurYMj3zLUtiY=; 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=P4PPbspEEUrnHnWPOeZlHc6cpSw2fPiAU23gE+/OSS7S+CeEYBJRX8qv2XVdQGwkj abVKOUFHepyJ4A5e4PNToO/f5AdTZhsGarNz8WsOqi5d5HaQwOACPGZbK5oUtqMXDF RUpIzMGa88Z/H3iBVaIMCDpKPh08dWZT0nz/r0A0= Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 0C9F43858406 for ; Tue, 10 Aug 2021 22:18:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0C9F43858406 Received: from Plymouth (unknown [IPv6:2a02:390:9086:0:9684:68da:ed3e:12ba]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id CC7FC81882; Tue, 10 Aug 2021 22:18:55 +0000 (UTC) Date: Tue, 10 Aug 2021 23:18:52 +0100 To: Simon Marchi Subject: Re: [PATCH v2 3/4] gdb: Have setter and getter callbacks for settings Message-ID: <20210810221852.bqbb6opnrlaqgob5@Plymouth> References: <20210808192302.3768766-1-lsix@lancelotsix.com> <20210808192302.3768766-4-lsix@lancelotsix.com> <727ab80a-c72d-960a-6d6e-a731aaa147dd@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <727ab80a-c72d-960a-6d6e-a731aaa147dd@polymtl.ca> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 10 Aug 2021 22:18:56 +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" > > @@ -231,25 +427,50 @@ struct base_setting > > gdb_assert (var_type_uses (this->m_var_type)); > > gdb_assert (!this->empty ()); > > I think this needs to call operator bool, like so: > > gdb_assert (*this); > > Otherwise, the assert fails when using a getter/setter (empty only > checks for m_var to be non-NULL, which is false when using > getter/setter). > Thanks for spotting this. The second assertion should only be on the branch that does not use the setter callback. I also realized that the 'set' method should use 'get_p () = v', which includes this assertion making the one you ran into redundant. > I don't find this notation super clear, it might be clearer if we could > call a named method instead of operator bool. Maybe "empty" could mean > "does not have a buffer nor getter/setter"? I think I got confused about what empty means when I rebased the patch and handled the conflicts from the previous iteration. This is a good indication that the naming is not as good as it could be. 'empty' refers to the fact that there is an underlying buffer register. I could just check 'm_var != nullptr' in 'get_p' and 'operator bool()'. This is a protected method not meant to be used outside of the class anyway. The 'bool()' operator is intended to check if a setting is valid, i.e. 'get' and 'set' can be called without a guarantied error. We could use something like 'valid ()' instead (or 'good ()' if we want to mimic iostream). Not that 'empty' would not work, I think I would prefer to read if (foo.valid ()) use (foo); over if (!foo.empty ()) use (foo); I tend to prefer the affirmative version over the double negation. I’ll change that for the next iteration. Lancelot. > > Simon