> -----Original Message-----
> From: Honnappa Nagarahalli [mailto:[email protected]]
> Sent: Tuesday, June 06, 2017 10:43 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <[email protected]>
> Cc: Github ODP bot <[email protected]>; [email protected]
> Subject: Re: [lng-odp] [PATCH API-NEXT v5 1/1] linux-generic: queue:
> modular queue interface
>
> On 6 June 2017 at 05:53, Savolainen, Petri (Nokia - FI/Espoo)
> <[email protected]> wrote:
> > Couple of general comments first:
> >
> > 1) Discussion about various versions of this patch has not reached the
> mailing list. That needs to be fixed in Github configuration...
> >
> > 2) Queue handle/qentry naming should be consistent over the entire
> patch.
> >
> > Today:
> > * "handle" refers to odp_queue_t
> > * "queue" or "qe" refers to *queue_entry_t when inside of queue.c
> > * "qentry" or "qe" refers to *queue_entry_t when outside of queue.c
> >
> > After this patch, the same should apply - especially for "handle". Try
> to minimize changes inside queue.c. When you add queue_t it should *not*
> be referred as "handle" (which is odp_queue_t), but something else - maybe
> consistently "queue_int". Otherwise, it's hard to follow which variable
> refers to odp_queue_t, queue_t or queue_entry_t.
> >
>
> The same should not apply because this patch has introduced another
> level of abstract data type and the code should reflect that. I do
> agree that consistent naming needs to be used. Following is my
> proposal:
>
> Looking at "outside of queue.c" (including odp_schedule*.c)
> 1) structure members of type 'odp_queue_t' and 'queue_t'. Naming is
> not consistent. The naming is according to the context/purpose of
> those members. I suggest that we do not rename these. Except members
> of type 'queue_t' can be suffixed with '_int'.
> 2) local variables and function arguments of type 'odp_queue_t' and
> 'queue_t'. These can be named 'queue' and 'queue_int' respectively
> There is no point in referring to these as 'handle' or 'handle_int',
> these do not convey anything outside queue.* files.
>
> Looking at "inside of queue.c"
> 1) anything that refers to *queue_entry_t can be called as 'qentry' or
> 'qe'
> 2) variables and function arguments of type 'odp_queue_t' and
> 'queue_t'. These can be named 'handle' and 'handle_int' respectively.
The internal interface may change later to pass through odp_queue_t instead of
queue_t on some occasions. Now it's just a copy-paste replacement of
qentry->xxx accesses. That in mind something that is today queue_t may change
to odp_queue_t later.
Conversions happen only in two levels:
* Outside of queue.c
* odp_queue_t <-> queue_t
==> between API handle and internal queue type (==> new interface)
* Inside of queue.c
* odp_queue_t <-> queue_entry_t
==> directly between API handle and queue_entry_t pointer (==>
queue_to_qentry())
* queue_t <-> queue_entry_t
==> between internal queue type and queue_entry_t pointer (==> just a cast)
Inside queue.c there's no need or benefit on two step conversion (odp_queue_t
-> queue_t -> queue_entry_t), since the last two types are defined/known by the
queue implementation itself.
queue.c
-------------
static inline queue_entry_t *queue_to_qentry(odp_queue_t handle)
{
uint32_t queue_id;
queue_id = queue_to_id(handle);
return get_qentry(queue_id);
}
static queue_t queue_from_ext(odp_queue_t handle)
{
return (queue_t)queue_to_qentry(handle);
}
static inline queue_entry_t *qentry_from_int(queue_t queue_int)
{
return (queue_entry_t *)(void *)(queue_int);
}
The direct handle to qentry should remain as a single operation
(queue_to_qentry() above). If you keep the same name, you save a lot of
unnecessary changes in queue.c. This patch should touch mainly outside of
queue.c (change direct queue entry accesses to functions).
-Petri
