netdev - Re: [PATCH net-next 1/1] wireguard: reformat to 100 column lines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 01 Jun 2020 04:12:14 -0700
From:   Joe Perches <joe@...ches.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>, netdev@...r.kernel.org,
        davem@...emloft.net
Subject: Re: [PATCH net-next 1/1] wireguard: reformat to 100 column lines

On Mon, 2020-06-01 at 00:29 -0600, Jason A. Donenfeld wrote:
> While this sort of change is typically viewed as "trivial", "cosmetic",
> or even "bikesheddy", I think there's a very serious argument to be made
> about the readability and comprehensibility of the code as a result.

Hey Jason.

I think a lot of these changes make a good deal of sense.

I certainly prefer:

> +static u8 common_bits(const struct allowedips_node *node, const u8 *key, u8 bits)
>  {
>  	if (bits == 32)
>  		return 32U - fls(*(const u32 *)node->bits ^ *(const u32 *)key);
>  	else if (bits == 128)
> -		return 128U - fls128(
> -			*(const u64 *)&node->bits[0] ^ *(const u64 *)&key[0],
> -			*(const u64 *)&node->bits[8] ^ *(const u64 *)&key[8]);
> +		return 128U - fls128(*(const u64 *)&node->bits[0] ^ *(const u64 *)&key[0],
> +				     *(const u64 *)&node->bits[8] ^ *(const u64 *)&key[8]);
>  	return 0;
>  }

(though the different uses of nodes->bits vs &nodes->bits[0]
 and node->key vs &node->key[0] in the two blocks is just odd.

whereas:

> -static bool prefix_matches(const struct allowedips_node *node, const u8 *key,
> -			   u8 bits)
> +static bool prefix_matches(const struct allowedips_node *node, const u8 *key, u8 bits)
>  {
> -	/* This could be much faster if it actually just compared the common
> -	 * bits properly, by precomputing a mask bswap(~0 << (32 - cidr)), and
> -	 * the rest, but it turns out that common_bits is already super fast on
> -	 * modern processors, even taking into account the unfortunate bswap.
> -	 * So, we just inline it like this instead.
> +	/* This could be much faster if it actually just compared the common bits properly, by
> +	 * precomputing a mask bswap(~0 << (32 - cidr)), and the rest, but it turns out that
> +	 * common_bits is already super fast on modern processors, even taking into account the
> +	 * unfortunate bswap.  So, we just inline it like this instead.

Reformatting comments just because you get more columns in the code
makes reading the comments a bit harder.

Newspaper columns are pretty narrow for a reason.

Please remember that left to right scanning of text, especially for
comments, is not particularly improved by longer lines.

cheers, Joe