From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org 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-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 10 Aug 2021 22:19:08 -0000 > > @@ -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