Eric Blake <ebl...@redhat.com> writes: > On Thu, Mar 16, 2023 at 08:13:15AM +0100, Markus Armbruster wrote: >> check_type() can check type names, arrays, and implicit struct types. >> Callers pass flags to select from this menu. This makes the function >> somewhat hard to read. Moreover, a few minor bugs are hiding in >> there, as we'll see shortly. >> >> Split it into check_type_name(), check_type_name_or_implicit(). Each > > You omitted check_type_name_or_array() in this summary
Oops! >> of them is a copy of the original specialized to a certain set of >> flags. >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> scripts/qapi/expr.py | 116 +++++++++++++++++++++++++------------------ >> 1 file changed, 67 insertions(+), 49 deletions(-) > >> >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index 59bdd86024..bc04bf34c2 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -333,62 +333,74 @@ def normalize_members(members: object) -> None: >> members[key] = {'type': arg} >> >> >> -def check_type(value: Optional[object], >> - info: QAPISourceInfo, >> - source: str, >> - allow_array: bool = False, >> - allow_dict: Union[bool, str] = False) -> None: > > There are few enough callers to see that they do indeed have exactly > one of (nearly) three call patterns. > >> - """ >> - Normalize and validate the QAPI type of ``value``. >> - >> - Python types of ``str`` or ``None`` are always allowed. >> - >> - :param value: The value to check. >> - :param info: QAPI schema source file information. >> - :param source: Error string describing this ``value``. >> - :param allow_array: >> - Allow a ``List[str]`` of length 1, which indicates an array of >> - the type named by the list element. >> - :param allow_dict: >> - Allow a dict. Its members can be struct type members or union >> - branches. When the value of ``allow_dict`` is in pragma >> - ``member-name-exceptions``, the dict's keys may violate the >> - member naming rules. The dict members are normalized in place. >> - >> - :raise QAPISemError: When ``value`` fails validation. >> - :return: None, ``value`` is normalized in-place as needed. >> - """ >> +def check_type_name(value: Optional[object], >> + info: QAPISourceInfo, source: str) -> None: > > check_type_name() replaces callers that relied on the default for > allow_array and allow_dict Yes. >> + if value is None: > > Loses out on the documentation. Not sure how much that matters to > you? You mean the doc string? I could copy and specialize it along with the code, but the new function is so simple... not sure it's worth explaining. >> + return >> + >> + if isinstance(value, str): >> + return >> + >> + if isinstance(value, list): >> + raise QAPISemError(info, "%s cannot be an array" % source) >> + >> + raise QAPISemError(info, "%s should be a type name" % source) >> + >> + >> +def check_type_name_or_array(value: Optional[object], >> + info: QAPISourceInfo, source: str) -> None: > > check_type_name_or_array() replaces all callers that passed > allow_array=True. Yes. >> if value is None: > > Another copy without documentation. Same doubts. >> return >> >> - # Type name >> if isinstance(value, str): >> return >> >> - # Array type >> if isinstance(value, list): >> - if not allow_array: >> - raise QAPISemError(info, "%s cannot be an array" % source) >> if len(value) != 1 or not isinstance(value[0], str): >> raise QAPISemError(info, >> "%s: array type must contain single type >> name" % >> source) >> return >> >> - # Anonymous type >> + raise QAPISemError(info, >> + "%s should be a type name" % source) >> >> - if not allow_dict: >> - raise QAPISemError(info, "%s should be a type name" % source) >> + >> +def check_type_name_or_implicit(value: Optional[object], >> + info: QAPISourceInfo, source: str, >> + parent_name: Optional[str]) -> None: > > And check_type_name_or_implicit replaces all callers that passed > allow_dict=str, where str is now the parent_name. Yes. > (Wow, that was an > odd overload of the parameter name - I like the split version better). It was less bad than what it replaced :) Commit 638c4af9310 (qapi: Clean up member name case checking) > ... >> @@ -560,10 +572,13 @@ def check_command(expr: QAPIExpression) -> None: >> rets = expr.get('returns') >> boxed = expr.get('boxed', False) >> >> - if boxed and args is None: >> - raise QAPISemError(expr.info, "'boxed': true requires 'data'") >> - check_type(args, expr.info, "'data'", allow_dict=not boxed) >> - check_type(rets, expr.info, "'returns'", allow_array=True) >> + if boxed: >> + if args is None: >> + raise QAPISemError(expr.info, "'boxed': true requires 'data'") >> + check_type_name(args, expr.info, "'data'") >> + else: >> + check_type_name_or_implicit(args, expr.info, "'data'", None) > > And this use of allow_dict was the weirdest, where it really does fit > better as calls into two separate functions. > > With the fixed commit message, and with or without more function docs, > > Reviewed-by: Eric Blake <ebl...@redhat.com> Thanks!