Hi Johan,
Thanks for the quick review! I've updated the too-long lines and taught
emacs to avoid mixing tabs/space in bluez code.
On 2013-07-24 07:57, johan.hedberg@xxxxxxxxx wrote:
> info->uuid.value is a union so I don't think it's safe to check any
> particular member before you've checked uuid.type. However, isn't what
> you want above to check that you have a BT_UUID16 or BT_UUID128? I.e.
> there should be no need to check for info->uuid.value.u16 directly.
Makes sense; note that the value check was in the original code, which I
didn't want to touch. In any case, I've changed it now.
> Be consistent with the coding style here: type casts should have a space
> between the cast and the variable.
> > typedef enum {
> > GATT_OPT_INVALID = 0,
> > GATT_OPT_CHR_UUID,
> > + GATT_OPT_CHR_BT_UUID_T,
> > GATT_OPT_CHR_PROPS,
> > GATT_OPT_CHR_VALUE_CB,
> > GATT_OPT_CHR_AUTHENTICATION,
> To be honest, I'm not 100% sure this is the best name for the new
> option, but I can't really think of anything much better either.
> Renaming the existing one to UUID_STR and the new one to UUID (like the
> old one was) would be one option, but it's one more patch and more code
> changes.
Indeed, couldn't come up with a better name myself :/... The old one
takes a uint16_t, so maybe we could call it UUID_16, and perhaps in the
future add UUID_STR for string-encoded 128-bit uuids.
Anyway, I'll send an updated patch.
Cheers,
Dirk.
--
Dirk-Jan C. Binnema Helsinki, Finland
e:djcb@xxxxxxxxxxxxxxx w:www.djcbsoftware.nl
pgp: D09C E664 897D 7D39 5047 A178 E96A C7A1 017D DA3C
--
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
