> On 16 Jun 2017, at 11:03, Peltonen, Janne (Nokia - FI/Espoo)
> <janne.pelto...@nokia.com> wrote:
>
>
> Honnappa Nagarahalli wrote:
>> On 12 June 2017 at 06:11, Petri Savolainen <petri.savolai...@linaro.org>
>> wrote:
>>> Clean up function and parameter naming after modular interface
>>> patch. Queue_t type is referred as "queue internal": queue_int or
>>> q_int. Term "handle" is reserved for API level handles (e.g.
>>> odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation.
>>>
>>
>> "queue_t" type should be referred to as "handle_int". "handle_int" is
>> clearly different from "handle".
>> If we look at the definition of "queue_t":
>>
>> typedef struct { char dummy; } _queue_t;
>> typedef _queue_t *queue_t;
>>
>> it is nothing but a definition of a handle. Why should it be called
>> with some other name and create confusion? Just like how odp_queue_t
>> is an abstract type, queue_t is also an abstract type. Just like how
>> odp_queue_t is a handle, queue_t is also a handle, albeit a handle
>> towards internal components.
>
> I do not see how calling variables of type queue_t handles instead
> of queue_int or q_int makes the call any clearer or less confusing.
> If the term handle is reserved for ODP API level handles, then I
> suppose this code should adhere to that. And 'handle_int' is not
> very descriptive as a variable name anyway.
>
>>> +static inline queue_entry_t *handle_to_qentry(odp_queue_t handle)
>>
>> Why is there a need to add this function? We already have
>> 'queue_from_ext' and 'qentry_from_int' which are a must to implement.
>> The functionality provided by 'handle_to_qentry' can be achieved from
>> these two functions. 'handle_to_qentry' is adding another layer of
>> wrapper. This adds to code complexity.
>
> There is a need to convert from handle to queue entry in quite many
> places in the code. Having a function for that makes perfect sense
> since it reduces code duplication and simplifies all the call sites
> that no longer need to know how the conversion is done.
>
> This is also how the code was before you changed it (unnecessarily,
> one might think), so this change merely brings back the old code
> structure (with a naming change).
>
>>> static odp_queue_type_t queue_type(odp_queue_t handle)
>>> {
>>> - return qentry_from_int(queue_from_ext(handle))->s.type;
>>> + return handle_to_qentry(handle)->s.type;
>>
>> No need to introduce another function.
>> qentry_from_int(queue_from_ext(handle)) clearly shows the current
>> status that there exists an internal handle. handle_to_qentry(handle)
>> hides that fact and makes code less readable. This comment applies to
>> all the instances of similar change.
>
> Hiding is good. Only handle_to_qentry() needs to know that the
> conversion is (currently) done through queue_t. I would argue that
> the code is more readable with handle_to_qentry() than with having
> to read the same conversion code all the time. An if the code ever
> changes so that the conversion is better done in another way, having
> handle_to_qentry() avoids the need to change all its call sites.
>
> Janne
>
>
I agree on all points with Janne.
-Matias
