From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id EJNvKcL2EmHQCwAAWB0awg (envelope-from ) for ; Tue, 10 Aug 2021 17:59:30 -0400 Received: by simark.ca (Postfix, from userid 112) id A666E1EDF7; Tue, 10 Aug 2021 17:59:30 -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 CE2861E813 for ; Tue, 10 Aug 2021 17:59:29 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 179993857C75 for ; Tue, 10 Aug 2021 21:59:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 179993857C75 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1628632769; bh=PUb+/rimJITZqsPFnD+MBnOj9asXBbU0AX8XjYFnIIM=; 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=UIbK1ZJoL5a+rUvX3dbkPMI9VE43PcwrFfy4AAyiYnlv38hDBdlTllQGkijl0Itdk +B0x/HLYakve3iKiS5XxiYXefHEufWD8e3tdPvMQd7wyDg9b9hzDx5f8XeZ84lvJ/e jMjC0QXaLu/6EoVhaRGfFU8R+BAXtwLKW0TuT8RY= Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 1229D3858003 for ; Tue, 10 Aug 2021 21:59:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1229D3858003 Received: from Plymouth (unknown [IPv6:2a02:390:9086:0:9684:68da:ed3e:12ba]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 2741F81882; Tue, 10 Aug 2021 21:58:59 +0000 (UTC) Date: Tue, 10 Aug 2021 22:58:52 +0100 To: Simon Marchi Subject: Re: [PATCH v2 1/4] gdb: Add typesafe getter/setter for cmd_list_element.var Message-ID: <20210810215852.vtd2vhnayefcxqi7@Plymouth> References: <20210808192302.3768766-1-lsix@lancelotsix.com> <20210808192302.3768766-2-lsix@lancelotsix.com> <2cc82d64-f2bd-1e7b-e91c-2b930a8dc75c@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2cc82d64-f2bd-1e7b-e91c-2b930a8dc75c@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 21:58:59 +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 Tue, Aug 10, 2021 at 08:22:31AM -0400, Simon Marchi wrote: > I'm forwarding some comments from Tony Tye, from AMD, who looked at the > patch, and had some minor comments. > > > @@ -230,7 +226,7 @@ struct cmd_list_element > > > > /* Pointer to variable affected by "set" and "show". Doesn't > > matter if type is not_set. */ > > - void *var = nullptr; > > + setting var; > > The comment should probably be updated, this is no longer a pointer. > Well, for now it contains a pointer, but later in the series it can > become a getter/setter too. So maybe just say "Setting affected by > ...". Indeed. I’ll update this and incorporate the other remarks you sent in your previous email. > > > diff --git a/gdb/command.h b/gdb/command.h > > index baf34401a07..644812c4d46 100644 > > --- a/gdb/command.h > > +++ b/gdb/command.h > > @@ -125,6 +125,164 @@ typedef enum var_types > > } > > var_types; > > > > +/* Return true if a setting of type VAR_TYPE is backed with type T. > > + > > + This function is left without definition intentionally. This template > > + is specialized for all valid types that are used to back var_types. > > + Therefore if one tries to instantiate this un-specialized template it > > + means the T parameter is not a type used to back a var_type and it is most > > + likely a programming error. */ > > +template > > +inline bool var_type_uses (var_types t); > > So what will happen if you try to use var_type_uses with an invalid > template type, link error? Tony suggested using a static_assert to have > a compile-time error instead, with an explanation. I suppose you would > define a body for this function and put a static_assert (false) in > there? With a 'standard' (development) build, this translate to compile-time error. I did not pay enough attention, but I implicitly relied on the fact that the configure script adds '-Werror' by default. Something should be in place to fail as early as possible (i.e. compile-time) whatever compiler options are in use. We cannot use a plain 'static_assert (false)' here, it will fail even if this template is never instantiated. All versions of g++/clang++ I just tried do trigger consistently: $CXX -c -x c++ - < void foo () { static_assert (false, "Invalid instantiation."); } EOT :3:4: error: static_assert failed "Invalid instantiation." { static_assert (false, "Invalid instantiation."); } ^ ~~~~~ 1 error generated. I could use something similar to this: template inline bool var_type_uses (var_types var_type ATTRIBUTE_UNUSED) { static_assert (sizeof(T) == 0, "Invalid type used to instantiate 'var_type_uses'. " "Use one of the types associated with enum var_types."); return false; } It is not the prettiest way of writing an assertion, but it is the only one I am aware of that can work in this situation. Would this be OK? Lancelot.