Re: [PATCH] Add WSP_VALUE_TYPE_TEXT support in wsp_decode_application_id()

[Jens Rehsack <jr_extern@vfnet.de>]

[-- Attachment #1: Type: text/plain, Size: 5132 bytes --]

On 21.05.2012 18:23, Ronald Tessier wrote:
> Hi Jens,
> 
> On 05/21/2012 12:54 PM, Jens Rehsack wrote:
>> From: Jens Rehsack<sno@NetBSD.org>
>>
>> ---
>>   src/wsputil.c |   43 ++++++++++++++++++++++++++-----------------
>>   1 Datei geändert, 26 Zeilen hinzugefügt(+), 17 Zeilen entfernt(-)
>>
>> diff --git a/src/wsputil.c b/src/wsputil.c
>> index 1b2b2b7..5611ed2 100644
>> --- a/src/wsputil.c
>> +++ b/src/wsputil.c
>> @@ -486,28 +486,37 @@ gboolean wsp_decode_application_id(struct
>> wsp_header_iter *iter,
>>                               const void **out_value)
>>   {
>>       const unsigned char *pdu_val = wsp_header_iter_get_val(iter);
>> -    unsigned int val;
>> -    unsigned int val_len;
>> -    unsigned int i;
>>
>> -    /*
>> -     * Well-known field values MUST be encoded using the
>> -     * compact binary formats
>> -     */
>> -    if (wsp_header_iter_get_val_type(iter) == WSP_VALUE_TYPE_SHORT) {
>> -        val = *pdu_val&  0x7f;
>> +    if (wsp_header_iter_get_val_type(iter) == WSP_VALUE_TYPE_TEXT) {
>> +        wsp_header_iter_get_val_len(iter);
>> +
>> +        if (out_value)
>> +            *out_value = pdu_val;
> 
> Why not just returning if the type is TEXT ?
> Something like :
> 
>     const unsigned char *pdu_val = wsp_header_iter_get_val(iter);
>     unsigned int val;
>     unsigned int val_len;
>     unsigned int i;
> 
> +    if (wsp_header_iter_get_val_type(iter) == WSP_VALUE_TYPE_TEXT) {
> +        if (out_value)
> +            *out_value = pdu_val;
> +
> +        return TRUE;
> +    }
> +
>     /*
>      * Well-known field values MUST be encoded using the
>      * compact binary formats
>      */
>     if (wsp_header_iter_get_val_type(iter) == WSP_VALUE_TYPE_SHORT) {
> 

I do not like several returns in a longer subroutine. The way
you format code makes code longer - fair enough, but for me to
long to introduce early return statements.

> I think it does the same thing but more readable (you don't break the 80
> col. limit).

I'm fine with your way to do it, too. But I wouldn't want my name being
assigned with it (because of the early return ...). I strongly
distinguish between readable and understandable, and I prefer the
understandable way.

How about:

diff --git a/src/wsputil.c b/src/wsputil.c
index 1b2b2b7..385acaa 100644
--- a/src/wsputil.c
+++ b/src/wsputil.c
@@ -490,13 +490,26 @@ gboolean wsp_decode_application_id(struct
wsp_header_iter *iter,
        unsigned int val_len;
        unsigned int i;

+       switch (wsp_header_iter_get_val_type(iter)) {
+       case WSP_VALUE_TYPE_TEXT:
+               if (out_value)
+                       *out_value = pdu_val;
+
+               break;
+
        /*
         * Well-known field values MUST be encoded using the
         * compact binary formats
         */
-       if (wsp_header_iter_get_val_type(iter) == WSP_VALUE_TYPE_SHORT) {
+       case WSP_VALUE_TYPE_SHORT:
                val = *pdu_val & 0x7f;
-       } else {
+
+               if (out_value)
+                       *out_value = get_text_entry(val, app_id);
+
+               break;
+
+       default:
                val_len = wsp_header_iter_get_val_len(iter);

                if (val_len > 2)
@@ -504,10 +517,13 @@ gboolean wsp_decode_application_id(struct
wsp_header_iter *iter,

                for (i = 0, val = 0; i < val_len && i < sizeof(val); i++)
                        val = (val << 8) | pdu_val[i];
+
+               if (out_value)
+                       *out_value = get_text_entry(val, app_id);
+
+               break;
        }

-       if (out_value)
-               *out_value = get_text_entry(val, app_id);

        return TRUE;
 }


> Best regards,
> 
> Ronald

Best regards,
Jens

>>       } else {
>> -        val_len = wsp_header_iter_get_val_len(iter);
>> +        unsigned int val;
>>
>> -        if (val_len>  2)
>> -            return FALSE;
>> +        /*
>> +         * Well-known field values MUST be encoded using the
>> +         * compact binary formats
>> +         */
>> +        if (wsp_header_iter_get_val_type(iter) ==
>> WSP_VALUE_TYPE_SHORT) {
>> +            val = *pdu_val&  0x7f;
>> +        } else  {
>> +            unsigned int val_len;
>> +            unsigned int i;
>>
>> -        for (i = 0, val = 0; i<  val_len&&  i<  sizeof(val); i++)
>> -            val = (val<<  8) | pdu_val[i];
>> -    }
>> +            val_len = wsp_header_iter_get_val_len(iter);
>>
>> -    if (out_value)
>> -        *out_value = get_text_entry(val, app_id);
>> +            if (val_len>  2)
>> +                return FALSE;
>> +
>> +            for (i = 0, val = 0; i<  val_len&&  i<  sizeof(val); i++)
>> +                val = (val<<  8) | pdu_val[i];
>> +        }
>> +
>> +        if (out_value)
>> +            *out_value = get_text_entry(val, app_id);
>> +    }
>>
>>       return TRUE;
>>   }
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono