Re: [PATCH 08/13] qapi: Assert output visitors see only valid enum values

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 4/23/20 11:00 AM, Markus Armbruster wrote:
>> output_type_enum() fails when *obj is not a valid value of the enum
>> type.  Should not happen.  Drop the check, along with its unit tests.
>> qapi_enum_lookup()'s assertion still guards.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   qapi/qapi-visit-core.c              |  9 -------
>>   tests/test-qobject-output-visitor.c | 39 -----------------------------
>>   tests/test-string-output-visitor.c  | 19 --------------
>>   3 files changed, 67 deletions(-)
>
> Nice cleanup.
>
> The commit message implies adding an assertion; but in reality, the
> change is deleting dead code (because we already have the assertion in
> place).  Maybe it's worth updating the subject?

I tried to say it in the body: "qapi_enum_lookup()'s assertion still
guards."  I could replace that by "This unmasks qapi_enum_lookup()'s
assertion."  Okay?  Better ideas?

> Reviewed-by: Eric Blake <address@hidden>
>
>> -    /*
>> -     * TODO why is this an error, not an assertion?  If assertion:
>> -     * delete, and rely on qapi_enum_lookup()
>> -     */
>> -    if (value < 0 || value >= lookup->size) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
>> -        return;
>> -    }
>
> The comment being deleted is what points out the assertion that
> already exists.

Thanks!