Re: [lmi] [lmi-commits] master a3a294f 4/7: Pass modifiable arguments by reference

[Vadim Zeitlin]

On Sun, 13 May 2018 19:45:54 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit a3a294f211f255fd0fc6de7be44ca7f3b339e7eb
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Pass modifiable arguments by reference
GC>     
GC>     Indirection impedes readability.

 Sorry, but I have to strongly disagree with this. On the contrary,
indirection is very helpful to see and hiding it serious impedes
comprehension of the code.

GC> @@ -1728,7 +1728,7 @@ class page_with_tabular_report
GC>          render_or_measure_extra_headers
GC>              (table_gen
GC>              ,interpolate_html
GC> -            ,&pos_y
GC> +            ,pos_y
GC>              ,output_mode
GC>              );

 Seeing a call like this in the code it's now not at all obvious that
"pos_y" is modified as the result of calling this function, whereas this
was quite clear before. The "&" here is not just some "syntactic noise", it
is valuable because it indicates that the parameter is modified. I don't
see what do we gain from removing it, but we definitely lose a lot in code
clarity.

 IMO output parameters should be passed by pointer or not at all. I.e. if
you absolutely don't want to revert this (and the next) commit(s), which I
think would be the best way to do, the function needs to be changed further
to return the new value of pos_y and take the argument by value instead of
modifying it. This would make things heavier than needed, but at least they
would (still) be clear. The current version is IMO the worst possible
approach.

 Regards,
VZ