linux-kernel - Re: [PATCH RFC 1/3] checkpatch: add verbose mode

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]

Message-ID: <9bfc9c21d93bb55419954114ed3a7e5cbdc84ddb.camel@perches.com>
Date:   Tue, 26 Jan 2021 12:11:09 -0800
From:   Joe Perches <joe@...ches.com>
To:     Dwaipayan Ray <dwaipayanray1@...il.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        lukas.bulwahn@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/3] checkpatch: add verbose mode

On Wed, 2021-01-27 at 00:05 +0530, Dwaipayan Ray wrote:
> Add a new verbose mode to checkpatch.pl to emit additional verbose
> test descriptions.
> 
> The verbose mode is optional and can be enabled by the flag
> --verbose.
> 
> The test descriptions are itself loaded from the checkpatch

descriptions are themselves, but themselves is unnecessary.

The verbose descriptions are read from Documentation/dev-tools/checkpatch.rst

> documentation file at Documentation/dev-tools/checkpatch.rst.
> The descriptions in the documentation are in a specified format
> enclosed within .. CHECKPATCH_START and .. CHECKPATCH_END labels.
> 
> This serves a dual purpose as an external documentation to checkpatch
> as well as enables flawless integration of the verbose mode.

Using 'flawless' when describing code or documentation generally isn't true.

> A subtle example of the format is as follows:

What is subtle about an example?

If there is something subtle about an example, there's also something
wrong with the example.

> Documentation/dev-tools/checkpatch.rst:
> 
> .. CHECKPATCH_START

Nak on the keyword uses.

This should really just parse the input file whenever TYPE is found
via some fixed format and save the verbose description after that.

Use .rst Field Lists instead, and ideally, keep the list in alphabetic
order or group by similar use.

https://docutils.sourceforge.io/docs/user/rst/quickref.html#field-lists

e.g.:

:LINE_SPACING:
	Vertical space is wasted given the limited number of lines an
	editor window can display when multiple blank lines are used.

:SPACING:
	Whitespace style used in the kernel sources is described in
	ref:`Documentation/process/Coding-Style.rst section 3.1.

:TRAILING_WHITESPACE:
	Trailing whitespace should always be removed.
	Some editors highlight the trailing whitespace and cause visual
	distractions when editing files.

etc...

> @@ -2185,6 +2235,11 @@ sub report {
>  		splice(@lines, 1, 1);
>  		$output = join("\n", @lines);
>  	}
> +
> +	if ($verbose && !$terse &&
> +	    exists $verbose_messages{$type}) {
> +		$output .= $verbose_messages{$type} . "\n\n";
> +	}
>  	$output = (split('\n', $output))[0] . "\n" if ($terse);

Don't use unnecessary multiple tests of the same object, just reorder
the code instead.  And also please use c-style function parentheses
rather than bare tests.

	if ($terse) {
		$output = ...
	} elsif ($verbose && exists($verbose_messages{$type})) {
		$output .= ...
	}