Hi Johan,
2011/11/12 Johan Hedberg <johan.hedberg@xxxxxxxxx>:
> Hi Santiago,
>
> On Thu, Nov 10, 2011, Santiago Carot-Nemesio wrote:
>> +struct measurement {
>> + gint16 exp;
>> + gint32 mant;
>> + guint64 time;
>> + gboolean suptime;
>> + gchar *unit;
>> + gchar *type;
>> + gchar *msmnt;
>
> Is msmnt supposed to be short for measurement? Could you just spell it
> out please since to me that short version is far from obvious. Or would
> "value" be a more suitable name?
>
Yes, that's supposed to be a kind of short for measurement, There's
not problem to me in changing the name. The reason why I set that name
was because I wanted to keep consistancy with the thermomether
documentation in which we called it measurement.
> Also, please avoid usage of g* types where you're not directly accessing
> some GLib API that expects them (even then it's mostly fine to go ahead
> with uint16_t, char, etc). I can see that there are other places in
> thermometer.c using the g types too which should be fixed in a separate
> patch.
>
Ok, I used only glib types because I was confused about when I should
use these or the other types, Having a look at some parts in BlueZ it
seemed a little mess, but now It's clear to me. Thank you for your
explanation,
If you dont mind I would preffer to send a separate patch fixing that
issue at the end of the new set in order to avoid too many conflicts
rebasing in my local tree.
>> +static gchar *temptype2str(uint8_t value)
>> +{
>> + switch (value) {
>> + case 1: return "Armpit";
>> + case 2: return "Body";
>> + case 3: return "Ear";
>> + case 4: return "Finger";
>> + case 5: return "Intestines";
>> + case 6: return "Mouth";
>> + case 7: return "Rectum";
>> + case 8: return "Toe";
>> + case 9: return "Tympanum";
>> + default:
>> + error("Temperature type %d reserved for future use", value);
>> + return NULL;
>> + };
>
> Please follow the coding style with switch statements. All those return
> statements should be on their own line. However, in this case I think
> it'd be just easier to make a lookup table for those values, i.e.
>
> const char *temp_type[] {
> "<unknown>",
> "Armpit",
> "Body",
> ...
>
> And then in the temptype2str function:
>
> if (type > 0 && val < G_N_ELEMENTS(temp_type))
> return temp_type[type];
>
>
> Johan
>
Ok, I'll redo these patches.
Thanks a lot for your comments.
Santiago
--
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
