On Mon, Dec 5, 2011 at 10:22 AM, Johan Hedberg <johan.hedberg@xxxxxxxxx> wrote:
> Hi Slawek,
>
> On Fri, Nov 25, 2011, Slawomir Bochenski wrote:
>> + * Author: Slawomir Bochenski <lkslawek@xxxxxxxxx>
>
> We don't have this convention in other files either so let's not break
> the consistency. We have the AUTHORS file and the git commit history to
> get this kind of info anyway.
>
>> +map_ap_t *map_ap_new(void)
>> +{
>> + return NULL;
>> +}
>> +
>> +void map_ap_free(map_ap_t *ap)
>> +{
>> +}
>> +
>> +map_ap_t *map_ap_parse(const uint8_t *buffer, uint32_t len)
>> +{
>> + return NULL;
>> +}
>> +
>> +GString *map_ap_output(map_ap_t *ap)
>> +{
>> + return NULL;
>> +}
>
> Is map_ap_output supposed to be the inverse of map_ap_parse? Why const
> uint8_t * for parse but GString * for output? Also (assuming that they
> really are supposed to be inverses) encode/decode would be clearer.
Yes, map_ap_output is inverse of map_ap_parse. uint8_t was chosen
after obex_get_apparam(). GString only as a convenience type that
stores together buffer and length. Would you prefer it to return size
and pass uint8_t ** to it (or reverse way)? Encode/decode is fine with
me.
>> +gboolean map_ap_get(map_ap_t *ap, enum map_ap_tag tag, void *value)
>> +{
>> + return FALSE;
>> +}
>> +
>> +gboolean map_ap_set(map_ap_t *ap, enum map_ap_tag tag, void *value)
>> +{
>> + return FALSE;
>> +}
>
> In a way having these generic get/set functions keeps the API small and
> clean, but then again there are only four possible types: 1, 2 and 4
> byte uints and strings. If there were get_u32, get_string, set_string,
> etc, you wouldn't e.g. need temporary variables to pass uints to the set
> functions but could pass constants directly and you'd also get the
> compiler to do some type checking for you (of course you still need to
> have an internal table to check which type a specific tag should have).
Make the choice here. For me it doesn't make a big difference.
> Btw, I assume that you've planned the API to take care of the big-endian
> conversion internally?
True. This is mentioned in documentation comment for map_ap_parse.
>> + MAP_AP_INVALID = 0x100,
>
> Where do you plan to use this value?
This is leftover of implementation detail. I use it as a guard for
array storing parameter types definitions.
> Since we now have unit/* in place it'd be nice if you could also add
> unit tests for this API as you go along implementing it.
Have they to be present already in this patch or can be added a bit later?
--
Slawomir Bochenski
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
