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

[Joe Perches]

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 .= ...
	}