Re: [PATCH V2 3/9] libxl_json: Introduce libxl__json_object_to_yajl_gen.

[Anthony PERARD <anthony.perard@citrix.com>]

On 09/25/2012 09:44 AM, Ian Campbell wrote:
> On Mon, 2012-09-17 at 19:22 +0100, Anthony PERARD wrote:
>> This function converts a libxl__json_object to yajl by calling every yajl_gen_*
>> function on a preallocated yajl_gen hand.
>>
>> This helps to integrate a json_object into an already existing yajl_gen tree.
>>
>> This function is used in a later patch.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  tools/libxl/libxl_internal.h |  3 +++
>>  tools/libxl/libxl_json.c     | 63 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 66 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 3c2dcaa..9c1482d 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> [...]
> 
>> +    }
>> +    case JSON_ERROR:
> 
> I can't see this being used anywhere, could it just be removed from the
> enum?

Yes.

>> +    case JSON_ANY:
> 
> Is JSON_ANY sort of like a JSON "void *"? Is it ever valid to be passed
> one here, IOW does this represent a coding error (==abort()) or a run
> time one (== log something)?

This value is only used as parameter of one function,
libxl__json_map_get, that return an object from the json_object tree
with the expected type. But sometime we don't expect any type so
JSON_ANY is used and allow "map_get" to return any type of object.

So JSON_ANY is not valid here and would probably be a coding error.

>> +    default:
> 
> If you skip this default case then some compilers will warn (a runtime
> error) if we forget to add a new JSON_FOO here.

I tried to remove the default, but then gcc believe that the function
would not return in some cases. But I did not add a return because it
felt weird to add a useless/never_executed return/abort.

But it seams better to remove the default case, so I will.

>> +        return -1;
>> +    }
>> +}

Thanks,

-- 
Anthony PERARD