Re: [PATCH net-next 0/4] mlx4 misc for 4.16

[David Miller <davem@davemloft.net>]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Thu, 28 Dec 2017 16:26:07 +0200

> This patchset contains misc cleanups and improvements
> to the mlx4 Core and Eth drivers.
> 
> In patches 1 and 2 I reduce and reorder the branches in the RX csum flow.
> In patch 3 I align the FMR unmapping flow with the device spec, to allow
>   a remapping afterwards.
> Patch 4 by Moni changes the default QoS settings so that a pause
>   frame stops all traffic regardless of its prio.
> 
> Series generated against net-next commit:
> 836df24a7062 net: hns3: hns3_get_channels() can be static

Series applied, thanks Tariq.

I can't say that that ipv6 ifdef you added in patch #1 is the nicest.

It's one thing to ifdef control entire blocks of code, or individual
values.

It's another thing to partially include pieces of code structure
such as closing parenthesis, arithmetic operators, and curly braces.
Two of those were included in the ifdef section.

For example, if we have:

	if (x & (IPV4_THING | IPV6_THING)) {

I don't want to see:

	if (x & (IPV4_THING |
#ifdef IPV6_TEST
		 IPV6_THING)) {
#else
		 0)) {
#endif

Those closing parenthesis and the openning curly brace are there
regardless of the CPP test, and duplicating them in both arms of
the CPP test only makes the code more confusing to read.

I can say I would prefer:

	if (x & (IPV4_THING |
#ifdef IPV6_TEST
		 IPV6_THING
#else
		 0
#endif
		 )) {

which is better.  However, best would be:

#ifdef IPV6_TEST
#define THING_MASK	IPV4_THING | IPV6_THING
#else
#define THING_MASK	IPV4_THING
#endif

	if (x & THING_MASK) {

is the best.