From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121792 invoked by alias); 18 Aug 2015 11:13:23 -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 121776 invoked by uid 89); 18 Aug 2015 11:13:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 18 Aug 2015 11:13:13 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 1299EC9D32; Tue, 18 Aug 2015 11:13:12 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t7IBDAoN007567; Tue, 18 Aug 2015 07:13:10 -0400 Message-ID: <55D31345.8010601@redhat.com> Date: Tue, 18 Aug 2015 11:13:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Yao Qi , Simon Marchi CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Replace some xmalloc-family functions with XNEW-family ones References: <1439848395-1869-1-git-send-email-simon.marchi@ericsson.com> <86614d3rsu.fsf@gmail.com> In-Reply-To: <86614d3rsu.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-08/txt/msg00458.txt.bz2 On 08/18/2015 09:49 AM, Yao Qi wrote: > Simon Marchi writes: > > Hi Simon, > Thanks for doing this. > >> This patch is part of the make-gdb-buildable-in-C++ effort. The idea is >> to change some calls to the xmalloc family of functions to calls to the >> equivalents in the XNEW family. This avoids adding an explicit cast, so >> it keeps the code a bit more readable. Some of them also map relatively >> well to a C++ equivalent (XNEW (struct foo) -> new foo), so it will be >> possible to do scripted replacements if needed. > > I am not against this patch, and I think this patch is useful to shorten > the code in some cases. However, I don't think we really need it to > make GDB buildable in C++, right? We can still use xmalloc in a C++ > program (xmalloc is still in use in GCC). Right. The issue is that in C++, we need to cast the result of xmalloc. E.g.: - struct foo *f = xmalloc (sizeof (struct foo)); + struct foo *f = (struct foo *) xmalloc (sizeof (struct foo)); We get away without the casts today because building in C++ mode currently forces -fpermissive (which is a temporary hack), which makes the missing cast a warning instead of an error. The XNEW-family of macros hide the cast away, and adds type-safety. E.g., this compiles and does the wrong thing: struct foo *f = (struct foo *) xmalloc (sizeof (struct bar)); This too, compiles and does the wrong thing: struct foo *f = (struct foo *) xmalloc (sizeof (struct foo *)); these don't compile: struct foo *f = XNEW (struct bar); struct foo *f = XNEW (struct foo *); So even without C++ in the picture, they could be considered an improvement, because they're typo-safer. The downside is the potential confusion to newcomers due to more obfuscation. We already use XNEW&co in the tree today though, so it's not really a new thing. > > XNEW/xmalloc -> new or struct -> class conversion should be done per > data structure rather than globally like this patch does. We only > convert C-inheritance data structures like breakpoint_ops, varobj, etc, > to C++ class, and leave the rest there unless it is necessary to change > them to C++ class. This is my personal understanding, and I'd like to > hear others thoughts. Agreed. We shouldn't go and do XNEW/xmalloc -> new/new[] all over the tree just for the sake of it. It will naturally happen sometimes that we'll refactor code and in the process xmalloc -> new happens, but I don't think that that should be a goal in itself. > >> >> I only changed calls that were obviously allocating memory for one or >> multiple "objects". Allocation of variable sizes (such as strings or >> buffer handling) will be for later (and won't use XNEW). >> >> - xmalloc (sizeof (struct foo)) -> XNEW (struct foo) >> - xmalloc (num * sizeof (struct foo)) -> XNEWVEC (struct foo, num) >> - xcalloc (1, sizeof (struct foo)) -> XCNEW (struct foo) >> - xcalloc (num, sizeof (struct foo)) -> XCNEWVEC (struct foo, num) >> - xrealloc (p, num * sizeof (struct foo) -> XRESIZEVEC (struct foo, p, num) >> - obstack_alloc (ob, sizeof (struct foo)) -> XOBNEW (ob, struct foo) >> - obstack_alloc (ob, num * sizeof (struct foo)) -> XOBNEWVEC (ob, struct foo, num) >> - alloca (sizeof (struct foo)) -> XALLOCA (struct foo) >> - alloca (num * sizeof (struct foo)) -> XALLOCAVEC (struct foo, num) >> >> Some instances of xmalloc followed by memset to zero the buffer were >> replaced by XCNEW or XCNEWVEC. > > I am not against this conversion. If we accept this change, are > xmalloc/xcalloc/... not allowed to use, and XNEW/XNEWVEC/... should be > used instead. The restriction is not necessary to me. I'm super fine with making XNEW not be a requirement, if people are OK with the extra casts. I was myself thinking of leaving these casts issues for one of the last steps of the C++ conversion in master (in the branch, it actually helps that it's one of the first in the series, as it gets the warnings out of the way), and just rely on the auto-insert-casts script. I actually had a couple (much smaller) patches that did xmalloc -> XNEW in my branch early on, but got rid of them in one of the latest rebases, as it seemed pointless when I was meaning to propose casts in many other places. :-P So IMO, kudos to Simon for being brave and doing all this. Seems like a good amount of work, and the result stands on its own, even without considering C++. The patch looks good to me, FWIW. Thanks, Pedro Alves