Re: [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper

[Vladimir Sementsov-Ogievskiy]

On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> This new annotation creates just a function wrapper that creates
> a new coroutine.

Actually, not just create, but create, start and wait for finish.. Maybe 
s/creates/starts/

> It assumes the caller is not a coroutine.
> It will be the default annotation to be used in the future.
>
> This is much better as c_w_mixed, because it is clear if the caller
> is a coroutine or not, and provides the advantage of automating
> the code creation. In the future all c_w_mixed functions will be
> substituted by co_wrapper.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---

[..]

>   
>   class FuncDecl:
> -    def __init__(self, return_type: str, name: str, args: str) -> None:
> +    def __init__(self, return_type: str, name: str, args: str,
> +                 variant: str) -> None:

I'd prefer mixed: bool parameter instead, to be more strict.

>           self.return_type = return_type.strip()
>           self.name = name.strip()
> +        self.struct_name = snake_to_camel(self.name)
>           self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
> +        self.create_only_co = True
> +
> +        if 'mixed' in variant:
> +            self.create_only_co = False

hmm, just

  self.create_only_co = 'mixed' not in variant

? And even better with boolean argument.

> +
> +        subsystem, subname = self.name.split('_', 1)
> +        self.co_name = f'{subsystem}_co_{subname}'
> +
> +        t = self.args[0].type
> +        if t == 'BlockDriverState *':
> +            bs = 'bs'
> +        elif t == 'BdrvChild *':
> +            bs = 'child->bs'
> +        else:
> +            bs = 'blk_bs(blk)'
> +        self.bs = bs
>   
>       def gen_list(self, format: str) -> str:
>           return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
> @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str:
>           return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
>   
>   
> -# Match wrappers declared with a co_wrapper_mixed mark
> -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
> +# Match wrappers declared with a co_wrapper mark
> +func_decl_re = re.compile(r'^int\s*co_wrapper'
> +                          r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'

Why you allow everything here?
I'd write just
 
  (?P<mixed>(_mixed)?)

or

  (?P<marker>co_wrapper(_mixed)?)

>                             r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
>                             r'\((?P<args>[^)]*)\);$', re.MULTILINE)
>   
> @@ -84,7 +103,8 @@ def func_decl_iter(text: str) -> Iterator:
>       for m in func_decl_re.finditer(text):
>           yield FuncDecl(return_type='int',
>                          name=m.group('wrapper_name'),
> -                       args=m.group('args'))
> +                       args=m.group('args'),
> +                       variant=m.group('variant'))
>   
>   
>   def snake_to_camel(func_name: str) -> str:
> @@ -97,24 +117,63 @@ def snake_to_camel(func_name: str) -> str:
>       return ''.join(words)
>   
>   
> +# Checks if we are already in coroutine

Please, do function documentation in doc-sting

> +def create_g_c_w(func: FuncDecl) -> str:

maybe, create_mixed_wrapper?

g_c_w refer to "generated_co_wrapper" and it was dropped from everywhere already

> +    name = func.co_name
> +    struct_name = func.struct_name
> +    return f"""\

[..]

> +
> +# Assumes we are not in coroutine, and creates one

move to doc-string

> +def create_coroutine_only(func: FuncDecl) -> str:

maybe, create_co_wrapper ?

Also, in the file functions that generate code are called gen_*,

so better gen_co_wrapper() and gen_co_wrapper_mixed()


> +    name = func.co_name
> +    struct_name = func.struct_name
> +    return f"""\
> +int {func.name}({ func.gen_list('{decl}') })
> +{{

[..]

>   
>   def gen_wrappers(input_code: str) -> str:


With at least comments for new functions turned into doc-strings:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

--
Best regards,
Vladimir