Eric Blake <ebl...@redhat.com> writes:
> On 9/10/19 1:37 AM, Markus Armbruster wrote:
>> The specification claims "Each expression that isn't an include
>> directive may be preceded by a documentation block", but the code also
>> rejects them for pragma directives. The code is correct. Fix the
>> specification.
>>
>> The specification reserves member names starting with 'has_', but the
>> code also reserves name 'u'. Fix the specification.
>
> Reservation of 'u' was done in 5e59baf9 (and claimed we could add a
> munge to q_u in the future if we ever needed a name 'u' after all).
Yes.
has_ could be munged away, too.
>> The specification claims "The string 'max' is not allowed as an enum
>> value". Untrue. Fix the specification. While there, delete the
>> naming advice, because it's redundant with the naming rules in section
>> "Schema overview"
>
> Used to be true; missed when commit 7fb1cf16 got rid of the collision.
Correct.
>> The specification claims "No branch of the union can be named 'max',
>> as this would collide with the implicit enum". Untrue. Fix the
>> specification.
>
> Fixed around the same time (although I didn't check if it was in the
> same commit)
>
>>
>> The specification claims "It is not allowed to name an event 'MAX',
>> since the generator also produces a C enumeration of all event names
>> with a generated _MAX value at the end." Untrue. Fix the
>> specification.
>
> And similar comment.
>
> I don't know if you want to do exact commit ids where all of these doc
> problems were introduced (because of code patches that lifted the
> limitations).
I'm (overly?) fond of git archeology myself, but I found these bugs
while fighting crocodiles in the swamp, so I couldn't indulge.
>> The specification claims "All branches of the union must be complex
>> types", but the code permits only struct types. The code is correct.
>> Fix the specification.
>>
>> The specification claims a command's return type "must be the string
>> name of a complex or built-in type, a one-element array containing the
>> name of a complex or built-in type" unless the command is in pragma
>> 'returns-whitelist'. The code does not permit built-in types. Fix
>> the specification.
>
> Umm:
>
> qapi/migration.json:{ 'command': 'query-migrate-cache-size', 'returns':
> 'int' }
>
> I don't know if we use an array of a built-in-type, but we definitely
> have unfortunate commands that return a non-JSON-object. [1]
>
>> A flat union definition avoids nesting on the wire, and specifies a
>> set of common members that occur in all variants of the union. The
>> 'base' key must specify either a type name (the type must be a
>> struct, not a union), or a dictionary representing an anonymous type.
>> -All branches of the union must be complex types, and the top-level
>> +All branches of the union must be struct types, and the top-level
>
> We have hit cases where it might have been nicer to permit a flat union
> whose branch is itself another flat union. But until we actually code
> that up to work, this is accurate.
>
>
>> @@ -578,8 +578,8 @@ The 'returns' member describes what will appear in the
>> "return" member
>> of a Client JSON Protocol reply on successful completion of a command.
>> The member is optional from the command declaration; if absent, the
>> "return" member will be an empty dictionary. If 'returns' is present,
>> -it must be the string name of a complex or built-in type, a
>> -one-element array containing the name of a complex or built-in type.
>> +it must be the string name of a complex type, or a
>> +one-element array containing the name of a complex type.
>> To return anything else, you have to list the command in pragma
>> 'returns-whitelist'. If you do this, the command cannot be extended
>> to return additional information in the future. Use of
>
> [1] Aha - it's 'returns-whitelist' that makes the difference. Okay,
> your wording change here makes sense: a built-in is NOT permitted UNLESS
> you whitelist it.
>
> Summary: you may want to improve the commit message with git
> archaeology, but the wording changes themselves make sense.
I'll see what I can do without too much effort.
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks!
