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: 5302 bytes --]

On 22.05.2012 10:09, Jens Rehsack wrote:
> 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;
>>>   }

What's the state?

/Jens