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

[Greg Chicares]

On 2018-05-14 14:17, Vadim Zeitlin wrote:
> 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.

Isn't that just an old C convention that's outmoded in C++? C didn't
have references, so a modifiable function argument had to be a pointer.
As a side effect, it was obvious at the call site whether a parameter
could be modified or not. That may have seemed handy, but it wasn't
actually useful--if it were, someone would suggest a cleaner way of
expressing it in C++, like writing 'const' at the call site:
  void foo(int);
  int x = 3;
  foo(x const); // this would be cleaner
or perhaps annotating the opposite case:
  void bar(int&);
  bar(x mutable); // indicate modifiability at call site
Indirection is a completely different concept; using it to indicate
non-const-ness just seems unnatural to me in C++.

Stroustrup & Sutter say:

  https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inout
| F.17: For “in-out” parameters, pass by reference to non-const

Also see:

  https://isocpp.org/wiki/faq/references#pointers-and-references
| Old line C programmers sometimes don’t like references since they
| provide reference semantics that isn’t explicit in the caller’s code.
| After some C++ experience, however, one quickly realizes this is a
| form of information hiding, which is an asset rather than a liability.

> 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.

Neither is it obvious--at the call site--how the other parameters are
passed (in three different ways), or which may be modified:
 - in-out: table_gen        is passed by reference
 - in    : interpolate_html is passed by const reference
 - in    : output_mode      is passed by value
or whether the 'this' pointer may be modified. The declaration is where
all that information is expressed.

But why should we care? If render_or_measure_extra_headers() modifies
'pos_y', so what? Writing a set of headers ought to move the position
where the next write will occur. That's no surprise.

> 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.

I see it in exactly the opposite way. I don't read '&' as suggesting that
the variable passed as an argument is to be modified (and that's why it
doesn't bother me that 'table_gen' above is passed by non-const reference).
When I see foo(a, &b, *c), my first thought is that 'b' and 'c' have weird
levels of indirection in order to make them work with some legacy API.

And that makes the code harder for me to understand or modify, because I
have to keep thinking about whether 'pos_y' is the vertical position, or
its address in memory:
 - whether I can use it as the vertical position, or must dereference it;
 - whether I can pass it directly, or must take its address.
If it can be either the value or its address, depending on context, then 
code that uses 'pos_y' in similar ways has to be written dissimilarly,
making comprehension needlessly hard. And of course a pointer might be null,
so that's one more thing that can go wrong.

Thus, in the original `git show a3a294f^:ledger_pdf_generator_wx.cpp`:

        table_gen.output_super_header
            ("Non-Guaranteed Values", column_mid_account_value, column_max
            ,&pos_y, output_mode);
[ampersand   ^ here]

        pos_y += table_gen.get_separator_line_height();
        table_gen.output_horz_separator
            (column_guar_account_value, column_separator_guar_non_guar
            ,pos_y, output_mode);
[no ampersand ^]

I didn't perceive a deliberate distinction between a modifiable 'pos_y' and
an unmodifiable one. I just saw a gratuitous inconsistency: an accidental
wrinkle that needs to be ironed out in order to make everything easy to read.

>  IMO output parameters should be passed by pointer or not at all.

Consider this example:

  #include <cstring>
  char a[100] = "Hello, ";
  char b[100] = "world!";
  std::strcat(a, b);

Both parameters are passed as pointers, but you can't tell which of them
is modified, unless of course you consult the declaration. Furthermore:

  char& rb = b[0];
  char c[100] = "Goodbye, ";
  std::strcat(c, &rb);

writing '&' at the call site doesn't necessarily make it clearer, because
the parameter passed with '&' isn't the one that's modified. IOW, the
'&-means-modifiable' convention isn't respected by the language, and can
even be contradicted, because indirection and constness are entirely
different concepts.

> 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.

But that would be inconsistent: in the render_or_measure_extra_headers()
case, the original declaration

    virtual void render_or_measure_extra_headers
        (illustration_table_generator& table_gen
        ,html_interpolator const&      interpolate_html
        ,int*                          pos_y
        ,oenum_render_or_only_measure  output_mode
        ) const

would have to be changed to pass 'table_gen' as a pointer, and the return
type would have to be std::pair<illustration_table_generator,int>. Then
we'd have to add code to assign from it:
  auto return_value = render_or_measure_extra_headers(...);
  table_gen = return_value.first;
  pos_y = return_value.seconds;
That's just too ugly to consider.

> The current version is IMO the worst possible
> approach.
Consider:
  (1) void foo(int& y);
  (2) int  foo(int  y); // returns modified value of 'y'
  (3) void foo(int* y);

I think you're saying your increasing preference order is
  1 << 2 < 3
but the isocpp guidelines cited above recommend the opposite:
  (1) for in-out parameters (like our 'pos_y') [F.17]
  (2) [without any argument] for out-only parameters [F.20]
They never even mention (3) in their comprehensive treatment:
  https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional
| F.15: Prefer simple and conventional ways of passing information
and the omission must be deliberate.

Making a variable a pointer doesn't indicate whether it's modifiable;
it indicates that nullptr is a valid value. References are safer: they
can't be uninitialized, can't be reseated, and, what is most important,
can never be null. And they're easier to read: you don't need '&' to
pass them or '*' to use them.

I've read through half a dozen of the articles found by this search:
  
https://www.google.com/search?q=c%2B%2B+output+parameter+pointer+or+reference+site:stackoverflow.com
and I really think that what you're advocating is not very popular.