From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94603 invoked by alias); 26 Nov 2018 17:18:24 -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 94584 invoked by uid 89); 26 Nov 2018 17:18:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=BAYES_00,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=201803, 2018-03, helping, angle X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Nov 2018 17:18:21 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id wAQHIEEe020241 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 26 Nov 2018 12:18:19 -0500 Received: by simark.ca (Postfix, from userid 112) id C366D1E992; Mon, 26 Nov 2018 12:18:14 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 4C53D1E473; Mon, 26 Nov 2018 12:18:12 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 26 Nov 2018 17:18:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 2/6] invoke_xmethod & array_view In-Reply-To: <8947e861-0974-f6b4-f5a8-a181df210e08@redhat.com> References: <20181015151115.6356-1-palves@redhat.com> <20181015151115.6356-3-palves@redhat.com> <245315f6-dd74-b533-f998-762dbf181a54@ericsson.com> <8947e861-0974-f6b4-f5a8-a181df210e08@redhat.com> Message-ID: <43be2c078bbfbcccd8ab3ec181a8813c@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00441.txt.bz2 On 2018-11-21 07:46, Pedro Alves wrote: > I changed it, though I'm not a big fan of that rule. > > As I wrote at , I > used to > prefer avoiding non-const reference output parameters too back in my > earlier C++ days, but I no longer do so much nowadays. > > The main point of avoiding non-const reference parameters is that > supposedly they make function callers unclearer, with no > visual indication that the function modified the argument. The > idea is that "&foo" instead of "foo" is that visual distinction. > But, that point can easily become moot because the visual distinction > can be easily lost with pointers too: > > // the usual argument is that using pointers for output parameters > shows > // clearly that bar is going to be modified: > function (foo, &bar); > > // but well, we often works with pointers, and if "bar" is a pointer, > // we don't get any visual clue anyway either: > function (foo, bar); > > // which suggests that what really helps is seeing the output > // variable nearby, suggesting to define it right before the > // function call that fills it in, and I would go as far > // as saying that clearer symbol names help even more. For e.g.: > B bar_out; > fill_bar (foo, bar_out); Well, I think that doing both (passing a pointer to the function, and defining the variable as close as possible) helps, they are not mutually exclusive. As for the name, it depends on the situation... if you are then going to pass bar_out as input to another function, then it's misleading. I think what helps the most is the fact that arguments passed by non-const references are highlighted red in my IDE :) > I think that symbol and function naming above is much more important > than "&bar" vs "bar". A function called "fill_bar" is clearly going > to write to its "bar_out" argument. As mentioned above, an "out" argument passed to a function call may be an "in" argument to the next one. I agree that good naming is important, but there is no one trick that will make the code "100%" readable. They all help a little bit towards that goal. > Also, a pointer can be null, while a reference can not. So > a reference parameter automatically implies that you can't pass > a NULL pointer to the function (which makes the function's > implementation a little bit clearer too), while with a pointer > parameter > you have to document that, and maybe assert it. With a reference, the > compiler is free to optimize accordingly (assume non-null), while with > a pointer, you have to use gcc's attribute nonnull if you want that, > which no one does. > > Also, for std::vector parameters in particular, passing by pointer > leads to uglier code in the function's implementation, like e.g., > > (*vec)[idx] = 0; > > instead of: > > vec[idx] = 0; > > We end up with a few instances like that in the series, though > admittedly > not that many, otherwise I think I'd add something like: > > auto &vec = *vec_param; > > at the top of the function and then use vec throughout. I acknowledge all these downsides. My opinion (as of today) is that they are reasonable trade-offs. I'm in the camp of "code is written once, read many times". I think that when writing it, it's not difficult to look up what the function you are calling expects (can a pointer be null). But when reading a function, if the little & sign can save a round trip to the called function's doc, I think it's worth it. > So in sum, I nowadays tend to look at reference vs parameter more from > the "pointer: optional, can be NULL", vs "reference: non-optional" > angle. > Though, given GDB's history, we definetely use pointers pervasively > in function parameters even when they're not supposed to be optional, > that's for sure. For "in" parameters, I think it's a no-brainer to do that. > Maybe we could come up with some middle ground rule, like always > putting in-out and output parameters last, and stressing to use > clear symbol names. Hmmm maybe, I would have to see it in practice to judge how feasible it is. > Anyway, I don't want to block on that discussion (my 1 month round trip > time not helping! :-D), so I did the change. Thanks! Note that I have introduced this rule kind of unilaterally after having no response when I proposed it: https://sourceware.org/ml/gdb-patches/2018-03/msg00145.html https://sourceware.org/ml/gdb-patches/2018-05/msg00450.html I thought it was reasonable to do so because it's not a huge commitment and easily reversible if people ended up disagreeing. So it's not as if it's set in stone. Simon