On 16 June 2017 at 03:03, Peltonen, Janne (Nokia - FI/Espoo)
<[email protected]> wrote:
>
> 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.
Should we then call variables of type odp_queue_t as 'queue'? I am not
suggesting variables of type queue_t to be called as 'handle', I am
suggesting that they should be called as 'handle_int'.
> If the term handle is reserved for ODP API level handles, then I
> suppose this code should adhere to that.
This code adheres to that. It is still calling variables of the type
odp_queue_t as 'handle'. As this patch introduces another type of
handle which is not exposed to the application, there is a need to
introduce another convention for variables of internal handle type,
hence it makes sense to call variables of internal type as
'handle_int'.
And 'handle_int' is not
> very descriptive as a variable name anyway.
Then, how are 'q_int' and 'queue_int' descriptive? What do they indicate?
>
>> > +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.
As I said, 'queue_from_ext' and 'qentry_from_int' are a must to
implement. Adding one more wrapper on top of them is duplication of
code, especially when this wrapper does not provide any extra
functionality. Why to add wrapper for the sake of adding wrappers?
qentry_from_int(queue_from_ext) makes it clear that there exists an
internal handle. 'handle_to_qentry' hides that fact and creates more
confusion, especially when rest of code base refers to
'qentry_from_int' and 'queue_from_ext'.
>
> 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).
>
This was changed because the function becomes redundant and there is
no need to have a wrapper function for the sake of having one.
>> > 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.
With the introduction of handle_to_qentry, one needs to understand
another extra function. Reading the same conversion clearly indicates
what is actually happening.
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.
The code should reflect the current situation correctly. Having the
function 'handle_to_qentry' is already changing all the call sites
today, to adopt to the new changes. Similarly, when things change in
the future, we will change the code at that time accordingly.
>
> Janne
>
>
