Re: [Openvpn-devel] [PATCH] route: cleanup codestyle and make code more readable

[David S. <op...@sf...>]

On 23/08/17 07:30, Antonio Quartulli wrote:
> This patch does not introduce any functional change.
> 
> The code in route.c seems to have been written in different
> periods by different people, without sticking to a clear
> codestyle. For this reason the code in this file in not
> consistent at all.
> 
> Clean it up by:
> - removing spaces from function invocations
> - cutting line longer than 80 chars (where possible)
> - moving function arguments on the same line when there is enough space
> - adding empty line between var declarations and code
> - adding empty line between code and final return
> - adding empty line to make the code less sticky and easier to parse
> 
> Signed-off-by: Antonio Quartulli <a...@un...>
> ---
> 
> Yes, this is a quite big patch. However, since we are planning  a big
> restructuring of the route.c file, it is better to take care of the
> style in a separate patch (this) so that later we don't need to mixup cleanups
> and refactoring.
> 
> Note that this patch is based on master plus the following patches:
> 
> - ensure function declarations are compiled with their definitions
> - fix a couple of typ0s in comments and strings
> - route: avoid definition of unused variables in certain configurations
> - convert *_inline attributes to bool
> - reformatting: fix style in crypto*.{c, h}
> - Allow learning iroutes with network made up of all 0s (only if netbits < 8)
> - ifconfig-ipv6(-push): allow using hostnames
> 
> 
> Applying this patch without the above, might lead to screams,
> natural disasters and endless nightmares.

I got it applying quite nicely (working my way through more patches
now).  And yes, I like that we clean up the coding style further in this
file.  But unfortunately, I'll have to say NAK in this round.

- Many places you replace spaces with tabs.
- There are several scenarios where our uncrustify config actually
  improves your patch further (see the attachment).
- And the contradictions like the ones below

> -static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es);
> +static void delete_route(struct route_ipv4 *r, const struct tuntap *tt,
> +			 unsigned int flags,
> +			 const struct route_gateway_info *rgi,
> +			 const struct env_set *es);

vs

>  static void
> -delete_route(struct route_ipv4 *r,
> -             const struct tuntap *tt,
> -             unsigned int flags,
> -             const struct route_gateway_info *rgi,
> -             const struct env_set *es)
> +delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags,
> +             const struct route_gateway_info *rgi, const struct env_set *es)

I think the change you do in the former one is also more readable than
squeezing everything into as few lines as possible, especially when
there's lots of arguments.

Our uncrustify config doesn't touch these details of function
declarations, as tun.c and route.c is fairly extreme in variations here.
 So we let that pass on the reformatting patch before the v2.4 release,
to take care of them manually, as we didn't spend much extra time
looking at more tweaks for uncrustify to make the result readable.  But
I'm not sure we documented our preferences on function declarations, I
don't recall that now.

Even though we are not united in the use of uncrustify after the
reformatting patches we did in December, I think it makes sense to at
least double check what uncrustify would change and consider those.  The
lesser the gap is to that result, the easier it will be to have a
consistent coding style over the complete code base.

For reference, the uncrustify command line I used was:

   $ uncrustify -c dev-tools/uncrustify.conf \
                 --no-backup -l C -p debug.uncr \
                 src/openvpn/route.c

-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc