Eric Blake <ebl...@redhat.com> writes: > On 9/24/19 8:28 AM, Markus Armbruster wrote: >> When we introduced the QAPISchema intermediate representation (commit >> ac88219a6c7), we took a shortcut: we left check_exprs() & friends >> alone instead of moving semantic checks into the >> QAPISchemaFOO.check(). check_exprs() still checks and reports errors, >> and the .check() assert check_exprs() did the job. There are a few >> gaps, though. >> >> QAPISchemaArrayType.check() neglects to assert the element type is not >> an array. Add the assertion. >> >> QAPISchemaObjectTypeVariants.check() neglects to assert the tag member >> is not optional. Add the assertion. >> >> It neglects to assert the tag member is not conditional. Add the >> assertion. >> >> It neglects to assert we actually have variants. Add the assertion. >> >> It asserts the variants are object types, but neglects to assert they >> don't have variants. Tighten the assertion. >> >> QAPISchemaObjectTypeVariants.check_clash() has the same issue. >> However, it can run only after .check(). Delete the assertion instead >> of tightening it. >> >> QAPISchemaAlternateType.check() neglects to assert the branch types >> don't conflict. Fixing that isn't trivial, so add just a TODO comment >> for now. It'll be resolved later in this series. > > I'm guessing you found these by deleting check_exprs() and seeing what > failed due to inadequate .check()
The first two or three I found by staring at the code. The remainder I found when I moved checks from check_exprs() to .check() [PATCH 16]: any check that doesn't replace an assertion means the assertion was missing. >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> scripts/qapi/common.py | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) > > Reviewed-by: Eric Blake <ebl...@redhat.com> Thanks!
