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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

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

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
>
> [...]
>
>

[-- Attachment #2: Type: text/html, Size: 4483 bytes --]