Re: [PATCH 16/16] qapi/expr.py: Use an expression checker dispatch table

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]

On Tue, Sep 22, 2020 at 05:13:13PM -0400, John Snow wrote:
> This enforces a type signature against all of the top-level expression
> check routines without necessarily needing to create some
> overcomplicated class hierarchy for them.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 69 ++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 74b2681ef8..cfd342aa04 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -31,8 +31,11 @@
>  structures and contextual semantic validation.
>  """
>  
> +from enum import Enum
>  import re
>  from typing import (
> +    Callable,
> +    Dict,
>      Iterable,
>      List,
>      MutableMapping,
> @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>      check_type(args, info, "'data'", allow_dict=not boxed)
>  
>  
> +class ExpressionType(str, Enum):
> +    INCLUDE = 'include'
> +    ENUM = 'enum'
> +    UNION = 'union'
> +    ALTERNATE = 'alternate'
> +    STRUCT = 'struct'
> +    COMMAND = 'command'
> +    EVENT = 'event'
> +
> +
> +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = {
> +    'enum': check_enum,
> +    'union': check_union,
> +    'alternate': check_alternate,
> +    'struct': check_struct,
> +    'command': check_command,
> +    'event': check_event,
> +}
> +
> +
>  def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>      """
>      Validate and normalize a list of parsed QAPI schema expressions. [RW]
> @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>          assert tmp is None or isinstance(tmp, QAPIDoc)
>          doc: Optional[QAPIDoc] = tmp
>  
> -        if 'include' in expr:
> -            continue
> -
> -        if 'enum' in expr:
> -            meta = 'enum'
> -        elif 'union' in expr:
> -            meta = 'union'
> -        elif 'alternate' in expr:
> -            meta = 'alternate'
> -        elif 'struct' in expr:
> -            meta = 'struct'
> -        elif 'command' in expr:
> -            meta = 'command'
> -        elif 'event' in expr:
> -            meta = 'event'
> +        for kind in ExpressionType:
> +            if kind in expr:
> +                meta = kind
> +                break
>          else:
>              raise QAPISemError(info, "expression is missing metatype")
>  
> +        if meta == ExpressionType.INCLUDE:
> +            continue
> +
>          name = cast(str, expr[meta])  # asserted right below:
> -        check_name_is_str(name, info, "'%s'" % meta)
> -        info.set_defn(meta, name)
> -        check_defn_name_str(name, info, meta)
> +        check_name_is_str(name, info, "'%s'" % meta.value)
> +        info.set_defn(meta.value, name)
> +        check_defn_name_str(name, info, meta.value)
>  
>          if doc:
>              if doc.symbol != name:
> @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
>              raise QAPISemError(info,
>                                 "documentation comment required")
>  
> -        if meta == 'enum':
> -            check_enum(expr, info)
> -        elif meta == 'union':
> -            check_union(expr, info)
> -        elif meta == 'alternate':
> -            check_alternate(expr, info)
> -        elif meta == 'struct':
> -            check_struct(expr, info)
> -        elif meta == 'command':
> -            check_command(expr, info)
> -        elif meta == 'event':
> -            check_event(expr, info)
> -        else:
> -            assert False, 'unexpected meta type'
> -
> -        check_if(expr, info, meta)
> +        _CHECK_FN[meta](expr, info)

I have to say the style of this line bothers me, but it's just that,
style. So,

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]