Re: [LTP] [PATCH] checkpatch: relax parenthesis style checks

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

> Hi Petr,

> > Hi Andrea, Cyril,

> > I found both rules useful (more readable and more consistent code). Can we put
> > it back? Over the years you will recognise on the code of single test that it
> > was written/modified by several people with a different styles. And that makes
> > it less readable.

> > * PARENTHESIS_ALIGNMENT check enforces space behind 'while' or 'if'.
> > i.e. instead of:
> > 	while(tst_fzsync_run_b(&fzsync_pair)) {
> > have:
> > 	while (tst_fzsync_run_b(&fzsync_pair)) {

> > cve-2014-0196.c mixes 'while()' and 'while ()'. Is it that hard to be
> > consistent on it?

> im not sure that rule is checking for spaces after the statements. that

I'm sorry, indeed, that is checked by SPACING which is still on.

> rule is avoiding stuff like:

> 	ruleset_fd = SAFE_LANDLOCK_CREATE_RULESET(ruleset_attr,
> 																						sizeof(struct tst_landlock_ruleset_attr_abi1), 0);

> because parenthesis should allign the attributes of the functions. that
> is super ugly when functions have really long names. Instead, this would
> make much more sense:


> 	ruleset_fd = SAFE_LANDLOCK_CREATE_RULESET(ruleset_attr,
> 							     sizeof(struct tst_landlock_ruleset_attr_abi1), 0);

Yes, it forces alignment after '('. Example of fix in the kernel:
https://lore.kernel.org/all/1421748570-14282-2-git-send-email-Emilian.Medve@Freescale.com/

 		pr_err("Could not register clock provider for node:%s\n",
-			 np->name);
+		       np->name);

I'd say it depends how your displays tabs (if tabs are in the sources, we have
.editorconfig, but not all not all developers have their editor set to use it).
In certain setup it does not look OK + the problem of inconsistency, but I'm ok
to ignore it.

> > * OPEN_ENDED_LINE asks for not ending line with '(' or '['.
> > i.e. instead of this:
> > 	ruleset_fd = TST_EXP_FD_SILENT(
> > 								   tst_syscall(__NR_landlock_create_ruleset, ruleset_attr,
> > 						   sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
> > have this:
> > 	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset, ruleset_attr,
> > 						   sizeof(struct tst_landlock_ruleset_attr_abi1), 0));

> you choose the right example, landlock testing suite has been updated to
> match this rule and now it has stuff like:

> 	apply_landlock_fs_layer(ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1),
> 				path_beneath_attr, MNTPOINT, LANDLOCK_ACCESS_FS_IOCTL_DEV);

In my personal preference is this above better ...

> instead of

> 	apply_landlock_fs_layer(
> 				ruleset_attr,
> 				sizeof(struct tst_landlock_ruleset_attr_abi1),
> 				path_beneath_attr,
> 				MNTPOINT,
> 				LANDLOCK_ACCESS_FS_IOCTL_DEV
> 	);

... than this. Why? Instead of 2 lines you have 7 lines. That would be ok, if we all
use portrait screens, but most of us code on landscape. (Too much scrolling.)

IMHO you introduced that style, which is still not widely used in the code.
Sooner or later we will have mix of 2 styles even in the single source file
which will be less readable.

> that is more clear what are the functions attributes.

Maybe this is is slightly more readable, but for me it does not pay off the
numer of extra lines. And functions with more than 3 parameters are hard to read
anyway (specially if passed parameters are just numbers).

> For the second one, maybe it's more like a personal preference, but the
> first rule is actually generating a lot of weird code when functions and
> attributes have long names, forcing to write long lines without staying
> into the 80-100 chars.

I'm ok with ignoring OPEN_ENDED_LINE, but I vote for putting back
PARENTHESIS_ALIGNMENT.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp