Re: [PATCH] net/nfp: add support of UDP fragmentation offload

[Bruce Richardson <bruce.richardson@intel.com>]

On Sun, Feb 18, 2024 at 11:05:35AM +0100, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Saturday, 17 February 2024 19.11
> > 
> > On Sat, 17 Feb 2024 19:02:30 +0100
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> > 
> > > Not formally... it follows the official DPDK Coding Style [1].
> > >
> > > [1]:
> > https://doc.dpdk.org/guides/contributing/coding_style.html#general
> > >
> > > > Should be:
> > > >
> > > > 	if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> > > > 	    (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> > > > 		goto clean_txd;
> > >
> > > This indentation style is mentioned as an alternative in the guide.
> > But the example in the guide also uses two tabs for a similar long
> > comparison.
> > >
> > > Personally, I also prefer the style suggested by Stephen, so we might
> > want to update the Coding Style. ;-)
> > 
> > 
> > The two tabs is an Intel thing, and I prefer the kernel, line up the
> > conditional style.
> 
> I prefer 4 space indentation, which is sufficient to notice the indentation. 8 spaces seems overkill to me, and quickly makes the lines too long.
> With the editor configured to show tab as 4 spaces, the kernel alignment style ends up with the same indentation for the condition and the code block:
> 
> if (a &&
>     b)
>     ctr++;
> 
> Whereas with the "tab as 4 spaces" editor configuration, the double indentation style clearly separates the continued condition from code block:
> 
> if (a &&
>         b)
>     ctr++;
> 

These two above are the main reasons I much prefer the double indent on
continuation, though I'd also add a third one: it means we don't have a mix
of tabs and spaces for indentation.

However, as stated already indent can be a matter of taste, and there will
be some disagreement about it. The existing coding standards document what
was done in the code base when they were written, and I don't think we
should look to change them. It's a bit annoying having 2 standards for
continuation rather than 1, but it's not exactly a free-for-all, and it's
not something that applies to every line, only to a small subset.

> On the other hand, complex conditions are easier readable when aligning logically instead of by a fixed number of tabs, e.g.:
> 
> if (a |
>     (b &
>      (c ^ d)) |
>     (e ^ f) |
>     g)
>     ctr++;
> 

Apart from the alignment of the increment at the end, yes, I admit it is a
little more readable. However, I also think that it's still pretty complex
even with the helpers!

> Placing the operators at the beginning also makes the code more readable:
> 
> if (a
>     | (b
>        & (c ^ d))
>     | (e ^ f)
>     | g)
>     ctr++;
> 

+1 to this. I think having operators at the beginning of lines is good. It
also makes it visually clearer that the indent is for line continuation.

> I guess that coding styles are mostly a matter of taste.
> 
> I wonder if any research into coding styles has reached any conclusions or recommendations, beyond mixing coding styles is bad for readability.
> 
> > We really should have a style that can be describe by clang format.
> > Other projects like VPP have a target that reformats the code and uses
> > one of the clang format templates.
> 
> Automated code formatting seems like a good idea.
> 

Yep. The trouble is that, if you don't do this from the start, the deltas
will be massive, and confusing in our git history.

/Bruce