Honnappa Nagarahalli wrote:
> On 12 June 2017 at 06:11, Petri Savolainen <[email protected]>
> 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
