[Swan-dev] style issue

[Andrew Cagney]

On Mon, 15 Mar 2021 at 16:41, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> This is about a matter of taste.  I'd like to understand why others
> don't agree with me.  I'm not just jumping in to "improve" the code.
>
> I like compact code.  It means that more functionality can be seen at
> once.
>
> That's one reason that I like C better than COBOL (there are many others).
>
> But this can be taken to far: I find APL pretty hard to read.  Maybe it is
> just that I haven't had enough exposure to internalize the set of
> operators.
>
> Anyway, I find this:
>
> bool selector_subnet_in(const ip_selector *lhs, const ip_selector *rhs)
> {
>         ip_subnet lhs_subnet = selector_subnet(*lhs);
>         ip_subnet rhs_subnet = selector_subnet(*rhs);
>         return subnet_in_subnet(lhs_subnet, rhs_subnet);
> }
>
> less readable than:
>
> bool selector_subnet_in(const ip_selector *lhs, const ip_selector *rhs)
> {
>         return subnet_in_subnet(selector_subnet(*lhs), selector_subnet(*rhs));
> }
>
> - shorter (fewer characters, fewer lines)
>
> - fewer variables
>
> - sometimes creating an intermediate variable helps the reader understand
>   what a subexpression means.  I don't see that applies in these cases.
>
> It may be that steppng through the first with gdb is more
> comprehensible than stepping through the second.  But how often are we
> stepping through code with gdb?

We're talking here about:

 * XXX: two hacks to get around .client not containing a proper
 * selector and instead needing to compare just the client's subnet.
 *
 * These are needed by code manipulating end.client because it is
 * serving double time.  It holding either:
 *
 * - the configured client subnet
 * - a connection's shunt

Up until about 10 minutes before this very post, the code had to look like:

  ip_subnet lhs_subnet = selector_subnet(lhs);
  ip_subnet rhs_subnet = selector_subnet(rhs);
  return subnet_in_subnet(&lhs_subnet, &rhs_subnet);

This is because all address functions took pointers and so
intermediate variables were a must!

I've now started changing this and a big motivator is so that I can
write address expressions more naturally, and just like you suggest.

However I'm not about to beautify the code while doing this:
- eliminating variables is what compilers live for
- it makes the diff easier to read
- it makes merging the next change easier (making these changes is
easy, testing is a pita)
(so please don't start beautifying this code).

Ah the irony.

> Here's another example:
>
> bool selector_subnet_is_address(const ip_selector *selector, const
> ip_address *address)
> {
>         if (address_is_unset(address) || selector_is_unset(selector)) {
>                 return false;
>         }
>         ip_subnet subnet = selector_subnet(*selector);
>         return subnet_eq_address(subnet, *address);
> }
>
> This looks simpler to me:
>
> bool selector_subnet_is_address(const ip_selector *selector, const
> ip_address *address)
> {
>         return  !address_is_unset(address) &&
>                 !selector_is_unset(selector) &&
>                 subnet_eq_address(selector_subnet(*selector), *address);
> }
>
> - it is shorter
>
> - there is no control-flow to analyze
>
> - there's no "false" to understand
>
> - a logical conjunction is easier to understand than a disjunction plus
>   a conjunction created by control flow.
>
> - a variable is eliminated
>
> - most of all: you can "read" the meaning of the function directly
>   rather than deriving it through the application of predicate
>   transformers.
>
> - one reason I use an algebraic language rather than assembly code is
>   so that I can write things like this concisely.
>
> What do you think?
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev