Re: [PATCH 1/1] docs: kernel-doc: fix parsing of function pointers

[Heinrich Schuchardt]

On 09/03/2018 09:00 PM, Jonathan Corbet wrote:
> On Mon,  3 Sep 2018 20:41:53 +0200
> Heinrich Schuchardt <xypron.glpk@xxxxxx> wrote:
> 
>> The same script kernel-doc is used by the U-Boot project.
> 
> Interesting, I didn't know that.  Please pass on my condolences :)
>>
>> kernel-doc fails to parse function definitions like the one below
>>
>> efi_status_t efi_create_event(uint32_t type, efi_uintn_t notify_tpl,
>> 			      void (EFIAPI *notify_function) (
>> 					struct efi_event *event,
>> 					void *context),
>> 			      void *notify_context, efi_guid_t *group,
>> 			      struct efi_event **event)
>> {
>>
>> due to the "EFIAPI" attribute preceding the function name.
> 
> This is a good description of the problem; a proper changelog should
> really say what the patch *does* about the problem too.  Especially
> since...
> 

Thanks for reviewing.

>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@xxxxxx>
>> ---
>>  scripts/kernel-doc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>> index 31a34ced55a3..597e3223b791 100755
>> --- a/scripts/kernel-doc
>> +++ b/scripts/kernel-doc
>> @@ -1381,7 +1381,7 @@ sub create_parameterlist($$$$) {
>>  	} elsif ($arg =~ m/\(.+\)\s*\(/) {

This matches anything that is not even remotely a function pointer, e.g.
"( )(".

>>  	    # pointer-to-function
>>  	    $arg =~ tr/#/,/;
>> -	    $arg =~ m/[^\(]+\(\*?\s*([\w\.]*)\s*\)/;

m/[^\(]+\(\*?\s*([\w\.]*)\s*\)/;
           ^
Here we allow for 0..1 asterixes.

If there is no asterix it is not a function pointer. Why should we care
for this case?

We do not allow for a space preceding the asterix, though it is
perfectly legal (though it is not the preferred formatting).

Probably we should start with collecting the different test cases that
the regex have to encompass. I already see the following possible
parameters:

int (*f)(int a)
int (* f)(int a)
int ( *f)(int a)
int ( * f)(int a)
int (FOO *f)(int a)
int (__attrib__((ms_abi)) *f)(int a)
int (*f(int b))(int a)

Which other cases do you see?

Best regards

Heinrich

>> +	    $arg =~ m/[^\(]+\([\w\s]*\*?\s*([\w\.]*)\s*\)/;
> 
> The meaning of this change doesn't just jump off the screen, even for
> folks who are accustomed to reading regexes.  It would be nice to explain
> what is actually being changed and what the expected new behavior is.  It
> appears to be consuming any normal text/white space prior to the optional
> "*".  Are you sure it doesn't overshoot and consume too much if the *
> isn't there?