Return: vs Returns:

Return: vs Returns:

[Matthew Wilcox]

This seems to be an extremely common mistake to make (indeed, almost
3000 occurrences of 'Returns:' vs 5300 occurrences of 'Return:').  Could
we have a checkpatch warning for it?

----- Forwarded message from Matthew Wilcox <willy@infradead.org> -----

On Wed, Jan 16, 2019 at 04:59:27PM +0000, Christophe Leroy wrote:
>  v3: Moved 'Returns:" comment after description.
>      Explained in the commit log why the function is defined static inline
> 
>  v2: Added "Returns:" comment and removed probe_user_address()

The correct spelling is 'Return:', not 'Returns:':

Return values
~~~~~~~~~~~~

The return value, if any, should be described in a dedicated section
named ``Return``.

----- End forwarded message -----

Re: Return: vs Returns:

[Mike Rapoport]

On Thu, Feb 07, 2019 at 05:59:24AM -0800, Matthew Wilcox wrote:
> 
> This seems to be an extremely common mistake to make (indeed, almost
> 3000 occurrences of 'Returns:' vs 5300 occurrences of 'Return:').
 
Add to that ~1000 '@return:'. 

But scripts/kernel-doc does not really care:

	} elsif ($newsection =~ m/^return?$/i) {
	    $newsection = $section_return;
	} elsif ($newsection =~ m/^\@return$/) {
	    # special: @return is a section, not a param description
	    $newsection = $section_return;
	}

> Could we have a checkpatch warning for it?

Does checkpatch checks the kernel-doc parts at all?

> ----- Forwarded message from Matthew Wilcox <willy@infradead.org> -----
> 
> On Wed, Jan 16, 2019 at 04:59:27PM +0000, Christophe Leroy wrote:
> >  v3: Moved 'Returns:" comment after description.
> >      Explained in the commit log why the function is defined static inline
> > 
> >  v2: Added "Returns:" comment and removed probe_user_address()
> 
> The correct spelling is 'Return:', not 'Returns:':
> 
> Return values
> ~~~~~~~~~~~~
> 
> The return value, if any, should be described in a dedicated section
> named ``Return``.
> 
> ----- End forwarded message -----
> 

-- 
Sincerely yours,
Mike.

Re: Return: vs Returns:

[Markus Heiser]

Am 07.02.19 um 16:30 schrieb Mike Rapoport:
> On Thu, Feb 07, 2019 at 05:59:24AM -0800, Matthew Wilcox wrote:
>>
>> This seems to be an extremely common mistake to make (indeed, almost
>> 3000 occurrences of 'Returns:' vs 5300 occurrences of 'Return:').
>   
> Add to that ~1000 '@return:'.
> 
> But scripts/kernel-doc does not really care:
> 
> 	} elsif ($newsection =~ m/^return?$/i) {
> 	    $newsection = $section_return;
> 	} elsif ($newsection =~ m/^\@return$/) {
> 	    # special: @return is a section, not a param description
> 	    $newsection = $section_return;
> 	}


Hi Mike, I only got this fragment of the thread, for me it is not absolutly
clear what the problem is .. I guess it is about the "Return" section in
kernel-doc comments, right?

The snippet from you above is the right point, it should work like it is
described here:

   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#return-values

doesn't it? Or did you just want a checkpatch ...

>> Could we have a checkpatch warning for it?
> 
> Does checkpatch checks the kernel-doc parts at all?

No.  I guess there are to many places to fail / to hard to put someone in
charge.  E.g. if you do include a single kernel-doc comment from a source all
kernel-docs in the source will be parsed and may produce (error/warning)
essages.  What we have, are some targets:

-linkcheckdocs
  check for broken external links (will connect to external hosts)

- refcheckdocs
   check for references to non-existing files under Documentation

-- Markus --

> 
>> ----- Forwarded message from Matthew Wilcox <willy@infradead.org> -----
>>
>> On Wed, Jan 16, 2019 at 04:59:27PM +0000, Christophe Leroy wrote:
>>>   v3: Moved 'Returns:" comment after description.
>>>       Explained in the commit log why the function is defined static inline
>>>
>>>   v2: Added "Returns:" comment and removed probe_user_address()
>>
>> The correct spelling is 'Return:', not 'Returns:':
>>
>> Return values
>> ~~~~~~~~~~~~
>>
>> The return value, if any, should be described in a dedicated section
>> named ``Return``.
>>
>> ----- End forwarded message -----
>>
> 

Re: Return: vs Returns:

[Mike Rapoport]

Hi Markus,

On Thu, Feb 07, 2019 at 04:58:17PM +0100, Markus Heiser wrote:
> Am 07.02.19 um 16:30 schrieb Mike Rapoport:
> >On Thu, Feb 07, 2019 at 05:59:24AM -0800, Matthew Wilcox wrote:
> >>
> >>This seems to be an extremely common mistake to make (indeed, almost
> >>3000 occurrences of 'Returns:' vs 5300 occurrences of 'Return:').
> >Add to that ~1000 '@return:'.
> >
> >But scripts/kernel-doc does not really care:
> >
> >	} elsif ($newsection =~ m/^return?$/i) {
> >	    $newsection = $section_return;
> >	} elsif ($newsection =~ m/^\@return$/) {
> >	    # special: @return is a section, not a param description
> >	    $newsection = $section_return;
> >	}
> 
> 
> Hi Mike, I only got this fragment of the thread, for me it is not absolutly
> clear what the problem is .. I guess it is about the "Return" section in
> kernel-doc comments, right?

Yeah, I think we can make kernel-doc more strict about it to start with.
 
> The snippet from you above is the right point, it should work like it is
> described here:
> 
>   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#return-values
> 
> doesn't it? Or did you just want a checkpatch ...
> 
> >>Could we have a checkpatch warning for it?
> >
> >Does checkpatch checks the kernel-doc parts at all?
> 
> No.  I guess there are to many places to fail / to hard to put someone in
> charge.  E.g. if you do include a single kernel-doc comment from a source all
> kernel-docs in the source will be parsed and may produce (error/warning)
> essages.  What we have, are some targets:
> 
> -linkcheckdocs
>  check for broken external links (will connect to external hosts)
> 
> - refcheckdocs
>   check for references to non-existing files under Documentation

Right, but these should be checked explicitly and I doubt many people do it
before submitting patches. OTOH, checkpatch is something that's widely used
and if it had verified the kernel-doc parts, more comments would be
following the convention.
 
> -- Markus --
> 
> >
> >>----- Forwarded message from Matthew Wilcox <willy@infradead.org> -----
> >>
> >>On Wed, Jan 16, 2019 at 04:59:27PM +0000, Christophe Leroy wrote:
> >>>  v3: Moved 'Returns:" comment after description.
> >>>      Explained in the commit log why the function is defined static inline
> >>>
> >>>  v2: Added "Returns:" comment and removed probe_user_address()
> >>
> >>The correct spelling is 'Return:', not 'Returns:':
> >>
> >>Return values
> >>~~~~~~~~~~~~
> >>
> >>The return value, if any, should be described in a dedicated section
> >>named ``Return``.
> >>
> >>----- End forwarded message -----
> >>
> >
> 

-- 
Sincerely yours,
Mike.

Re: Return: vs Returns:

[Joe Perches]

On Thu, 2019-02-07 at 18:18 +0200, Mike Rapoport wrote:
> Hi Markus,
> 
> On Thu, Feb 07, 2019 at 04:58:17PM +0100, Markus Heiser wrote:
> > Am 07.02.19 um 16:30 schrieb Mike Rapoport:
> > > On Thu, Feb 07, 2019 at 05:59:24AM -0800, Matthew Wilcox wrote:
> > > > This seems to be an extremely common mistake to make (indeed, almost
> > > > 3000 occurrences of 'Returns:' vs 5300 occurrences of 'Return:').
> > > Add to that ~1000 '@return:'.
[]
> > > > Could we have a checkpatch warning for it?
> > > 
> > > Does checkpatch checks the kernel-doc parts at all?
> > 
> > No.  I guess there are to many places to fail / to hard to put someone in
> > charge.  E.g. if you do include a single kernel-doc comment from a source all
> > kernel-docs in the source will be parsed and may produce (error/warning)
> > essages.  What we have, are some targets:
> > 
> > -linkcheckdocs
> >  check for broken external links (will connect to external hosts)
> > 
> > - refcheckdocs
> >   check for references to non-existing files under Documentation
> 
> Right, but these should be checked explicitly and I doubt many people do it
> before submitting patches. OTOH, checkpatch is something that's widely used
> and if it had verified the kernel-doc parts, more comments would be
> following the convention.

It's not clear to me what you are asking checkpatch to do here.

It may be reasonable for checkpatch to invoke kernel-doc on some
portion of a patch, but I'm not sure how valuable it will be.

Re: Return: vs Returns:

[Markus Heiser]

Am 07.02.19 um 17:18 schrieb Mike Rapoport:
>>> Does checkpatch checks the kernel-doc parts at all?
>> No.  I guess there are to many places to fail / to hard to put someone in
>> charge.  E.g. if you do include a single kernel-doc comment from a source all
>> kernel-docs in the source will be parsed and may produce (error/warning)
>> essages.  What we have, are some targets:
>>
>> -linkcheckdocs
>>   check for broken external links (will connect to external hosts)
>>
>> - refcheckdocs
>>    check for references to non-existing files under Documentation
> Right, but these should be checked explicitly and I doubt many people do it
> before submitting patches. OTOH, checkpatch is something that's widely used
> and if it had verified the kernel-doc parts, more comments would be
> following the convention.
>   

I'am with you, but I do not have any clue how to solve this Gordian Knot
faithful and without massive collateral damage / sorry :|

The only thing I know, we have the -none option:

$ ./scripts/kernel-doc -none ./include/media/cec.h
./include/media/cec.h:51: warning: Function parameter or member 'lock' not 
described in 'cec_devnode'

But this is nothing more than noise if the patch does not touch cec_devnode.
And there is another problem I see, if we want to check refs ...

 >> -linkcheckdocs
 >>   check for broken external links (will connect to external hosts)
 >>
 >> - refcheckdocs
 >>    check for references to non-existing files under Documentation

The refs are solved late in the sphinx build process when .rst files and
kernel-doc comments come together .. so we need sphinx for checkpatch,
I gues this is a no-go (?)

-- Markus --

Re: Return: vs Returns:

[Matthew Wilcox]

On Thu, Feb 07, 2019 at 09:31:20AM -0800, Joe Perches wrote:
> It's not clear to me what you are asking checkpatch to do here.
> 
> It may be reasonable for checkpatch to invoke kernel-doc on some
> portion of a patch, but I'm not sure how valuable it will be.

I was just hoping to match:

 * Returns:

Or to quote it properly for regexes ...

^ +\* *Returns:

(I think ...)

I can't see that matching C or assembler.

Re: Return: vs Returns:

[Joe Perches]

On Thu, 2019-02-07 at 09:34 -0800, Matthew Wilcox wrote:
> On Thu, Feb 07, 2019 at 09:31:20AM -0800, Joe Perches wrote:
> > It's not clear to me what you are asking checkpatch to do here.
> > 
> > It may be reasonable for checkpatch to invoke kernel-doc on some
> > portion of a patch, but I'm not sure how valuable it will be.
> 
> I was just hoping to match:
> 
>  * Returns:
> 
> Or to quote it properly for regexes ...
> 
> ^ +\* *Returns:
> 
> (I think ...)
> 
> I can't see that matching C or assembler.

checkpatch doesn't attempt to enforce any formatting standard
on kernel-doc comments.

There doesn't seem to be much standardization for kernel-doc
in the first place.

Just for the * return: case:

$ git grep -P -i '^\s*\*\s*returns?\s*:' -- '*.[ch]' | \
  grep -P -oh -i '\*\s*returns?\s*:' | \
  sort | uniq -c | sort -rn
   5153 * Return:
   2534 * Returns:
   1077 * RETURN:
    358 * RETURNS:
    173 *	RETURNS:
    171 * returns:
    153 * return:
    148 * Return :
     72 * Returns :
     61 *	Returns:
     37 *  Returns:
     30 *  returns:
     27 *  return:
     22 *	Return:
     20 * Returns  :
     19 *  Return:
     15 *  RETURNS:
     11 *           return:
      6 * return :
      6 *	return:
      5 * returns :
      3 *Returns:
      3 * Returns	:
      3 * 	returns:
      2 *RETURNS:
      2 *     Returns:
      2 *      Returns:
      2 *	returns:
      2 * RETURN :
      2 *      Return:
      2 * 	Return:
      2 *	return :
      2 *		return:
      1 *   RETURNS:
      1 * RETURNs:
      1 *   Returns:
      1 *    Returns:
      1 * 	Returns:
      1 *  RETURN:
      1 *   Return:
      1 *    Return:
      1 * return   :

I think standarization is more something that scripts/kernel-doc
could or should do.

Re: Return: vs Returns:

[Markus Heiser]

Am 07.02.19 um 18:50 schrieb Joe Perches:
> On Thu, 2019-02-07 at 09:34 -0800, Matthew Wilcox wrote:
>> On Thu, Feb 07, 2019 at 09:31:20AM -0800, Joe Perches wrote:
>>> It's not clear to me what you are asking checkpatch to do here.
>>>
>>> It may be reasonable for checkpatch to invoke kernel-doc on some
>>> portion of a patch, but I'm not sure how valuable it will be.
>>
>> I was just hoping to match:
>>
>>   * Returns:
>>
>> Or to quote it properly for regexes ...
>>
>> ^ +\* *Returns:
>>
>> (I think ...)
>>
>> I can't see that matching C or assembler.
> 
> checkpatch doesn't attempt to enforce any formatting standard
> on kernel-doc comments.
> 
> There doesn't seem to be much standardization for kernel-doc
> in the first place.
> 
> Just for the * return: case:
> 
> $ git grep -P -i '^\s*\*\s*returns?\s*:' -- '*.[ch]' | \
>    grep -P -oh -i '\*\s*returns?\s*:' | \
>    sort | uniq -c | sort -rn
>     5153 * Return:
>     2534 * Returns:
>     1077 * RETURN:
>      358 * RETURNS:
>      173 *	RETURNS:
>      171 * returns:
>      153 * return:
>      148 * Return :
>       72 * Returns :
>       61 *	Returns:
>       37 *  Returns:
>       30 *  returns:
>       27 *  return:
>       22 *	Return:
>       20 * Returns  :
>       19 *  Return:
>       15 *  RETURNS:
>       11 *           return:
>        6 * return :
>        6 *	return:
>        5 * returns :
>        3 *Returns:
>        3 * Returns	:
>        3 * 	returns:
>        2 *RETURNS:
>        2 *     Returns:
>        2 *      Returns:
>        2 *	returns:
>        2 * RETURN :
>        2 *      Return:
>        2 * 	Return:
>        2 *	return :
>        2 *		return:
>        1 *   RETURNS:
>        1 * RETURNs:
>        1 *   Returns:
>        1 *    Returns:
>        1 * 	Returns:
>        1 *  RETURN:
>        1 *   Return:
>        1 *    Return:
>        1 * return   :
> 
> I think standarization is more something that scripts/kernel-doc
> could or should do.

BTW: kernel-doc parser accept 'return' and 'returns':

     } elsif ($newsection =~ m/^return?$/i) {
         $newsection = $section_return;

Is there really a need to be standardize this? From the list above I think
there are only a few where it fails.

-- Markus --

Re: Return: vs Returns:

[Joe Perches]

On Thu, 2019-02-07 at 18:58 +0100, Markus Heiser wrote:
> Am 07.02.19 um 18:50 schrieb Joe Perches:
> > On Thu, 2019-02-07 at 09:34 -0800, Matthew Wilcox wrote:
> > > On Thu, Feb 07, 2019 at 09:31:20AM -0800, Joe Perches wrote:
> > > > It's not clear to me what you are asking checkpatch to do here.
> > > > 
> > > > It may be reasonable for checkpatch to invoke kernel-doc on some
> > > > portion of a patch, but I'm not sure how valuable it will be.
> > > 
> > > I was just hoping to match:
> > > 
> > >   * Returns:
> > > 
> > > Or to quote it properly for regexes ...
> > > 
> > > ^ +\* *Returns:
> > > 
> > > (I think ...)
> > > 
> > > I can't see that matching C or assembler.
> > 
> > checkpatch doesn't attempt to enforce any formatting standard
> > on kernel-doc comments.
> > 
> > There doesn't seem to be much standardization for kernel-doc
> > in the first place.
> > 
> > Just for the * return: case:
> > 
> > $ git grep -P -i '^\s*\*\s*returns?\s*:' -- '*.[ch]' | \
> >    grep -P -oh -i '\*\s*returns?\s*:' | \
> >    sort | uniq -c | sort -rn
> >     5153 * Return:
> >     2534 * Returns:
> >     1077 * RETURN:
> >      358 * RETURNS:
> >      173 *	RETURNS:
> >      171 * returns:
> >      153 * return:
> >      148 * Return :
> >       72 * Returns :
> >       61 *	Returns:
> >       37 *  Returns:
> >       30 *  returns:
> >       27 *  return:
> >       22 *	Return:
> >       20 * Returns  :
> >       19 *  Return:
> >       15 *  RETURNS:
> >       11 *           return:
> >        6 * return :
> >        6 *	return:
> >        5 * returns :
> >        3 *Returns:
> >        3 * Returns	:
> >        3 * 	returns:
> >        2 *RETURNS:
> >        2 *     Returns:
> >        2 *      Returns:
> >        2 *	returns:
> >        2 * RETURN :
> >        2 *      Return:
> >        2 * 	Return:
> >        2 *	return :
> >        2 *		return:
> >        1 *   RETURNS:
> >        1 * RETURNs:
> >        1 *   Returns:
> >        1 *    Returns:
> >        1 * 	Returns:
> >        1 *  RETURN:
> >        1 *   Return:
> >        1 *    Return:
> >        1 * return   :
> > 
> > I think standarization is more something that scripts/kernel-doc
> > could or should do.
> 
> BTW: kernel-doc parser accept 'return' and 'returns':
> 
>      } elsif ($newsection =~ m/^return?$/i) {
>          $newsection = $section_return;

That regex doesn't look like it does accept returns.
That looks like it accepts either retur or return.
I believe that would need to be

	$newsection =~ m/returns?$/i

> Is there really a need to be standardize this?

I generally doubt it.

Re: Return: vs Returns:

[Markus Heiser]

Am 07.02.19 um 18:58 schrieb Markus Heiser:

> 
> BTW: kernel-doc parser accept 'return' and 'returns':
> 
>      } elsif ($newsection =~ m/^return?$/i) {
>          $newsection = $section_return;

Sorry wrong C&P, here is the one from the source:

	} elsif ($newsection =~ m/^returns?$/i) {
	    $newsection = $section_return;
	} elsif ($newsection =~ m/^\@return$/) {

-- Markus --

> 
> Is there really a need to be standardize this? From the list above I think
> there are only a few where it fails.
> 
> -- Markus --

Re: Return: vs Returns:

[Jani Nikula]

On Thu, 07 Feb 2019, Joe Perches <joe@perches.com> wrote:
> On Thu, 2019-02-07 at 18:58 +0100, Markus Heiser wrote:
>> Am 07.02.19 um 18:50 schrieb Joe Perches:
>> > On Thu, 2019-02-07 at 09:34 -0800, Matthew Wilcox wrote:
>> > > On Thu, Feb 07, 2019 at 09:31:20AM -0800, Joe Perches wrote:
>> > > > It's not clear to me what you are asking checkpatch to do here.
>> > > > 
>> > > > It may be reasonable for checkpatch to invoke kernel-doc on some
>> > > > portion of a patch, but I'm not sure how valuable it will be.
>> > > 
>> > > I was just hoping to match:
>> > > 
>> > >   * Returns:
>> > > 
>> > > Or to quote it properly for regexes ...
>> > > 
>> > > ^ +\* *Returns:
>> > > 
>> > > (I think ...)
>> > > 
>> > > I can't see that matching C or assembler.
>> > 
>> > checkpatch doesn't attempt to enforce any formatting standard
>> > on kernel-doc comments.
>> > 
>> > There doesn't seem to be much standardization for kernel-doc
>> > in the first place.
>> > 
>> > Just for the * return: case:
>> > 
>> > $ git grep -P -i '^\s*\*\s*returns?\s*:' -- '*.[ch]' | \
>> >    grep -P -oh -i '\*\s*returns?\s*:' | \
>> >    sort | uniq -c | sort -rn
>> >     5153 * Return:
>> >     2534 * Returns:
>> >     1077 * RETURN:
>> >      358 * RETURNS:
>> >      173 *	RETURNS:
>> >      171 * returns:
>> >      153 * return:
>> >      148 * Return :
>> >       72 * Returns :
>> >       61 *	Returns:
>> >       37 *  Returns:
>> >       30 *  returns:
>> >       27 *  return:
>> >       22 *	Return:
>> >       20 * Returns  :
>> >       19 *  Return:
>> >       15 *  RETURNS:
>> >       11 *           return:
>> >        6 * return :
>> >        6 *	return:
>> >        5 * returns :
>> >        3 *Returns:
>> >        3 * Returns	:
>> >        3 * 	returns:
>> >        2 *RETURNS:
>> >        2 *     Returns:
>> >        2 *      Returns:
>> >        2 *	returns:
>> >        2 * RETURN :
>> >        2 *      Return:
>> >        2 * 	Return:
>> >        2 *	return :
>> >        2 *		return:
>> >        1 *   RETURNS:
>> >        1 * RETURNs:
>> >        1 *   Returns:
>> >        1 *    Returns:
>> >        1 * 	Returns:
>> >        1 *  RETURN:
>> >        1 *   Return:
>> >        1 *    Return:
>> >        1 * return   :
>> > 
>> > I think standarization is more something that scripts/kernel-doc
>> > could or should do.
>> 
>> BTW: kernel-doc parser accept 'return' and 'returns':
>> 
>>      } elsif ($newsection =~ m/^return?$/i) {
>>          $newsection = $section_return;
>
> That regex doesn't look like it does accept returns.

Yeah that copy-paste is from who knows where, kernel-doc DTRT.

> That looks like it accepts either retur or return.
> I believe that would need to be
>
> 	$newsection =~ m/returns?$/i
>
>> Is there really a need to be standardize this?
>
> I generally doubt it.

I ran a regex similar to yours (there's also "@returns?:") way back when
adding Sphinx support, and my conclusion was trying to standardize this
is futile. Sure we can document and encourage one (I chose "Return:"
based on the grep popularity contest) but trying to change everything is
just unnecessary busywork.

However, kernel-doc does standardize the *output* for more uniform and
nicer looking results.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: Return: vs Returns:

[Mike Rapoport]

On Thu, Feb 07, 2019 at 06:33:34PM +0100, Markus Heiser wrote:
> 
> Am 07.02.19 um 17:18 schrieb Mike Rapoport:
> >>>Does checkpatch checks the kernel-doc parts at all?
> >>No.  I guess there are to many places to fail / to hard to put someone in
> >>charge.  E.g. if you do include a single kernel-doc comment from a source all
> >>kernel-docs in the source will be parsed and may produce (error/warning)
> >>essages.  What we have, are some targets:
> >>
> >>-linkcheckdocs
> >>  check for broken external links (will connect to external hosts)
> >>
> >>- refcheckdocs
> >>   check for references to non-existing files under Documentation
> >Right, but these should be checked explicitly and I doubt many people do it
> >before submitting patches. OTOH, checkpatch is something that's widely used
> >and if it had verified the kernel-doc parts, more comments would be
> >following the convention.
> 
> I'am with you, but I do not have any clue how to solve this Gordian Knot
> faithful and without massive collateral damage / sorry :|
> 
> The only thing I know, we have the -none option:
> 
> $ ./scripts/kernel-doc -none ./include/media/cec.h
> ./include/media/cec.h:51: warning: Function parameter or member 'lock' not
> described in 'cec_devnode'
> 
> But this is nothing more than noise if the patch does not touch cec_devnode.
> And there is another problem I see, if we want to check refs ...

Well, the case when a patch changes function parameters but forgets to
update the kernel-doc part is particularly annoying.
I believe it's possible to match function parameter changes with the
corresponding kernel-doc changes (or lack of them).
 
> >> -linkcheckdocs
> >>   check for broken external links (will connect to external hosts)
> >>
> >> - refcheckdocs
> >>    check for references to non-existing files under Documentation
> 
> The refs are solved late in the sphinx build process when .rst files and
> kernel-doc comments come together .. so we need sphinx for checkpatch,
> I gues this is a no-go (?)
> 
> -- Markus --
> 

-- 
Sincerely yours,
Mike.