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
