From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81634 invoked by alias); 4 Apr 2017 18:40:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 81620 invoked by uid 89); 4 Apr 2017 18:40:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=liked X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Apr 2017 18:40:00 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E3B7EC0885EC for ; Tue, 4 Apr 2017 18:40:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E3B7EC0885EC Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com E3B7EC0885EC Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 861AF784C0; Tue, 4 Apr 2017 18:39:59 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 13/18] -Wwrite-strings: Wrap PyGetSetDef for construction with string literals References: <1491326751-16180-1-git-send-email-palves@redhat.com> <1491326751-16180-14-git-send-email-palves@redhat.com> X-URL: https://sergiodj.net Date: Tue, 04 Apr 2017 18:40:00 -0000 In-Reply-To: <1491326751-16180-14-git-send-email-palves@redhat.com> (Pedro Alves's message of "Tue, 4 Apr 2017 18:25:46 +0100") Message-ID: <877f30dnk0.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00052.txt.bz2 On Tuesday, April 04 2017, Pedro Alves wrote: > Unfortunately, PyGetSetDef's 'name' and 'doc' members are 'char *' > instead of 'const char *', meaning that in order to list-initialize > PyGetSetDef arrays using string literals requires writing explicit > 'char *' casts. For example: > > static PyGetSetDef value_object_getset[] = { > - { "address", valpy_get_address, NULL, "The address of the value.", > + { (char *) "address", valpy_get_address, NULL, > + (char *) "The address of the value.", > NULL }, > - { "is_optimized_out", valpy_get_is_optimized_out, NULL, > - "Boolean telling whether the value is optimized " > + { (char *) "is_optimized_out", valpy_get_is_optimized_out, NULL, > + (char *) "Boolean telling whether the value is optimized " > "out (i.e., not available).", > NULL }, > - { "type", valpy_get_type, NULL, "Type of the value.", NULL }, > - { "dynamic_type", valpy_get_dynamic_type, NULL, > - "Dynamic type of the value.", NULL }, > - { "is_lazy", valpy_get_is_lazy, NULL, > - "Boolean telling whether the value is lazy (not fetched yet\n\ > + { (char *) "type", valpy_get_type, NULL, > + (char *) "Type of the value.", NULL }, > + { (char *) "dynamic_type", valpy_get_dynamic_type, NULL, > + (char *) "Dynamic type of the value.", NULL }, > + { (char *) "is_lazy", valpy_get_is_lazy, NULL, > + (char *) "Boolean telling whether the value is lazy (not fetched yet\n\ > from the inferior). A lazy value is fetched when needed, or when\n\ > the \"fetch_lazy()\" method is called.", NULL }, > {NULL} /* Sentinel */ > > We have 20 such arrays, and I first wrote a patch that fixed all of > them like that... It's not pretty, and I can post it if people want > to see it. > > One way to make these a bit less ugly would be add a new macro that > hides the casts, like: > > #define GDBPY_GSDEF(NAME, GET, SET, DOC, CLOSURE) \ > { (char *) NAME, GET, SET, (char *) DOC, CLOSURE } > > and then use it like: > > static PyGetSetDef value_object_getset[] = { > GDBPY_GSDEF ("address", valpy_get_address, NULL, > "The address of the value.", NULL), > GDBPY_GSDEF ("is_optimized_out", valpy_get_is_optimized_out, NULL, > "Boolean telling whether the value is optimized ", NULL), > {NULL} /* Sentinel */ > }; > > But since we have C++11, which gives us constexpr and list > initialization, I thought of a way that requires no changes where the > arrays are initialized: > > We add a new type that extends PyGetSetDef (called gdb_PyGetSetDef), > and add constexpr constructors that accept const 'name' and 'doc', and > then list/aggregate initialization simply "calls" these matching > constructors instead. > > I put "calls" in quotes, because given "constexpr", it's all done at > compile time, and there's no overhead either in binary size or at run > time. In fact, we get identical binaries, before/after this change. > > I'm a bit undecided whether to change the places that create > PyGetSetDef arrays to explicitly name gdb_PyGetSetDef as type, like: > > - static PyGetSetDef value_object_getset[] = { > + static gdb_PyGetSetDef value_object_getset[] = { > > or go the > > #define PyGetSetDef gdb_PyGetSetDef > > way as we do for the other Python API fixes. This commit takes the > latter approach, but I'll change it if people prefer the other way. I liked the approach. As with the other Python patch, I'm more inclined to use an explicit "gdb_PyGetSetDef" everywhere, just to be clear that we are using our own version of it. This can save time when debugging. Otherwise, the rest is fine by me. > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves > > * python/python-internal.h (gdb_PyGetSetDef): New type. > (PyGetSetDef): New define. > --- > gdb/python/python-internal.h | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h > index 55efd75..8fc89cd 100644 > --- a/gdb/python/python-internal.h > +++ b/gdb/python/python-internal.h > @@ -286,6 +286,38 @@ gdb_PySys_SetPath (const GDB_PYSYS_SETPATH_CHAR *path) > > #define PySys_SetPath gdb_PySys_SetPath > > +/* Wrap PyGetSetDef to allow convenient construction with string > + literals. Unfortunately, PyGetSetDef's 'name' and 'doc' members > + are 'char *' instead of 'const char *', meaning that in order to > + list-initialize PyGetSetDef arrays with string literals (and > + without the wrapping below) would require writing explicit 'char *' > + casts. Instead, we extend PyGetSetDef and add onstexpr > + constructors that accept const 'name' and 'doc', hiding the ugly > + casts here in a single place. */ > + > +struct gdb_PyGetSetDef : PyGetSetDef > +{ > + constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter set_, > + const char *doc_, void *closure_) > + : PyGetSetDef {const_cast (name_), get_, set_, > + const_cast (doc_), closure_} > + {} > + > + /* Alternative constructor that allows omitting the closure in list > + initialization. */ > + constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter set_, > + const char *doc_) > + : gdb_PyGetSetDef {name_, get_, set_, doc_, NULL} > + {} > + > + /* Constructor for the sentinel entries. */ > + constexpr gdb_PyGetSetDef (std::nullptr_t) > + : gdb_PyGetSetDef { NULL, NULL, NULL, NULL, NULL } > + {} > +}; > + > +#define PyGetSetDef gdb_PyGetSetDef > + > /* In order to be able to parse symtab_and_line_to_sal_object function > a real symtab_and_line structure is needed. */ > #include "symtab.h" > -- > 2.5.5 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/