Re: [PATCH 2/6] invoke_xmethod & array_view

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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 <https://gcc.gnu.org/ml/gcc/2018-07/msg00190.html>, 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