Re: [PATCH v6 11/11] qapi: make 'if' condition strings simple identifiers

[Marc-André Lureau]

On Tue, Aug 3, 2021 at 5:35 PM Markus Armbruster <armbru@redhat.com> wrote:

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Change the 'if' condition strings to be C-agnostic and be simple
> identifiers.

This is less general, and that's okay, we're doing it for a purpose.
But the commit message should mention / explain all this.

ok

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Tested-by: John Snow <jsnow@redhat.com>
> ---

[...]

> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index f8718e201b..0c718e43c9 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -218,7 +218,7 @@ def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>      if not ifcond:
>          return ''
>      if isinstance(ifcond, str):
> -        return ifcond
> +        return 'defined(' + ifcond + ')'
> 
>      oper, operands = next(iter(ifcond.items()))
>      if oper == 'not':
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index d2bd52c49f..d355cbc8c1 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -281,10 +281,10 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> 
>      def _check_if(cond: Union[str, object]) -> None:
>          if isinstance(cond, str):
> -            if not cond.strip():
> +            if not cond.isidentifier():

This accepts *Python* identifiers:

    $ python
    Python 3.9.6 (default, Jul 16 2021, 00:00:00)
    [...]
    >>> 'André'.isidentifier()
    True

These may or may not work for the languages we generate.  Wouldn't
restricting identifiers to something like /[A-Z][A-Z0-9_]*/ make more
sense?

yes, works for me

 

>                  raise QAPISemError(
>                      info,
> -                    "'if' condition '%s' of %s makes no sense"
> +                    "'if' condition '%s' of %s is not a valid identifier"
>                      % (cond, source))
>              return
> 
> diff --git a/tests/qapi-schema/alternate-branch-if-invalid.err b/tests/qapi-schema/alternate-branch-if-invalid.err
> index d384929c51..03bad877a3 100644
> --- a/tests/qapi-schema/alternate-branch-if-invalid.err
> +++ b/tests/qapi-schema/alternate-branch-if-invalid.err
> @@ -1,2 +1,2 @@
>  alternate-branch-if-invalid.json: In alternate 'Alt':
> -alternate-branch-if-invalid.json:2: 'if' condition ' ' of 'data' member 'branch' makes no sense
> +alternate-branch-if-invalid.json:2: 'if' condition ' ' of 'data' member 'branch' is not a valid identifier

[...]