diff mbox

doxygen: Add a buffer module

Message ID 1409710691-22816-1-git-send-email-mike.holmes@linaro.org
State Rejected
Headers show

Commit Message

Mike Holmes Sept. 3, 2014, 2:18 a.m. UTC
Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
---

This creates a new section in the documentaion to group everything
related to buffers. It appears to make things much more acessable
although it needs to have some real description added to the section.

 platform/linux-generic/include/api/odp_buffer.h      | 6 +++++-
 platform/linux-generic/include/api/odp_buffer_pool.h | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

Comments

Stuart Haslam Sept. 4, 2014, 10:10 a.m. UTC | #1
On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> ---
> 
> This creates a new section in the documentaion to group everything
> related to buffers. It appears to make things much more acessable
> although it needs to have some real description added to the section.
> 

Looks better to me. It also allows more freedom within the implementation
to place things in different header files and still have the documentation
look the same.

Small nit, I think it would be better to use defgroup and ingroup, rather
than addtogroup, to avoid any confusion about where the documentation for
that group should go (what happens if you document multiple addtogroups?).
vkamensky Sept. 4, 2014, 2:41 p.m. UTC | #2
On 4 September 2014 03:10, Stuart Haslam <stuart.haslam@arm.com> wrote:
> On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>> ---
>>
>> This creates a new section in the documentaion to group everything
>> related to buffers. It appears to make things much more acessable
>> although it needs to have some real description added to the section.
>>
>
> Looks better to me. It also allows more freedom within the implementation
> to place things in different header files and still have the documentation
> look the same.

If implementations will have freedom to place things in different headers,
how one app source will work with all of them? Which header it would
include? I am not against fancy doxygen syntax, but mapping ODP
symbol to header file name should be normative part of ODP. Of course,
such mapping could be supported with set of implementation defined
headers that are internally included by one that constitute ODP API.

Thanks,
Victor

> Small nit, I think it would be better to use defgroup and ingroup, rather
> than addtogroup, to avoid any confusion about where the documentation for
> that group should go (what happens if you document multiple addtogroups?).
>
> --
> Stuart.
>
>>  platform/linux-generic/include/api/odp_buffer.h      | 6 +++++-
>>  platform/linux-generic/include/api/odp_buffer_pool.h | 6 ++++++
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/platform/linux-generic/include/api/odp_buffer.h b/platform/linux-generic/include/api/odp_buffer.h
>> index d8577fd..4f348e4 100644
>> --- a/platform/linux-generic/include/api/odp_buffer.h
>> +++ b/platform/linux-generic/include/api/odp_buffer.h
>> @@ -21,7 +21,10 @@ extern "C" {
>>
>>  #include <odp_std_types.h>
>>
>> -
>> +/**
>> + * @addtogroup buffer
>> + * @{
>> + */
>>
>>  /**
>>   * ODP buffer
>> @@ -83,6 +86,7 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>>   */
>>  void odp_buffer_print(odp_buffer_t buf);
>>
>> +/** @} */
>>
>>  #ifdef __cplusplus
>>  }
>> diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h
>> index fe88898..dd0ac43 100644
>> --- a/platform/linux-generic/include/api/odp_buffer_pool.h
>> +++ b/platform/linux-generic/include/api/odp_buffer_pool.h
>> @@ -23,6 +23,11 @@ extern "C" {
>>  #include <odp_std_types.h>
>>  #include <odp_buffer.h>
>>
>> +/**
>> + * @addtogroup buffer ODP Buffers and buffer Pools
>> + * @{
>> + */
>> +
>>  /** Maximum queue name lenght in chars */
>>  #define ODP_BUFFER_POOL_NAME_LEN  32
>>
>> @@ -99,6 +104,7 @@ void odp_buffer_free(odp_buffer_t buf);
>>   */
>>  odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
>>
>> +/** @} */
>>
>>  #ifdef __cplusplus
>>  }
>> --
>> 1.9.1
>>
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Stuart Haslam Sept. 4, 2014, 3:01 p.m. UTC | #3
On Thu, Sep 04, 2014 at 03:41:17PM +0100, Victor Kamensky wrote:
> On 4 September 2014 03:10, Stuart Haslam <stuart.haslam@arm.com> wrote:
> > On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> >> ---
> >>
> >> This creates a new section in the documentaion to group everything
> >> related to buffers. It appears to make things much more acessable
> >> although it needs to have some real description added to the section.
> >>
> >
> > Looks better to me. It also allows more freedom within the implementation
> > to place things in different header files and still have the documentation
> > look the same.
> 
> If implementations will have freedom to place things in different headers,
> how one app source will work with all of them? Which header it would
> include? I am not against fancy doxygen syntax, but mapping ODP
> symbol to header file name should be normative part of ODP. Of course,
> such mapping could be supported with set of implementation defined
> headers that are internally included by one that constitute ODP API.
> 
> Thanks,
> Victor
> 

This has come up before and I think the intention has always been that
the application should just include odp.h and the implementation can do
whatever it likes with header files beyond that (except perhaps that
they need to be named odp_*.h).

Personally though, I would rather the file names were fixed, and I did
wonder when writing that comment whether this decision should be
revisited.
Mike Holmes Sept. 4, 2014, 3:07 p.m. UTC | #4
On 4 September 2014 06:10, Stuart Haslam <stuart.haslam@arm.com> wrote:

> On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> > ---
> >
> > This creates a new section in the documentaion to group everything
> > related to buffers. It appears to make things much more acessable
> > although it needs to have some real description added to the section.
> >
>
> Looks better to me. It also allows more freedom within the implementation
> to place things in different header files and still have the documentation
> look the same.
>
> Small nit, I think it would be better to use defgroup and ingroup, rather
> than addtogroup, to avoid any confusion about where the documentation for
> that group should go (what happens if you document multiple addtogroups?).
>


We can make it explicit, I was thinking that it was easier to manage with
the looser association.
multiple add groups just concatenate without a warning.




> --
> Stuart.
>
> >  platform/linux-generic/include/api/odp_buffer.h      | 6 +++++-
> >  platform/linux-generic/include/api/odp_buffer_pool.h | 6 ++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/platform/linux-generic/include/api/odp_buffer.h
> b/platform/linux-generic/include/api/odp_buffer.h
> > index d8577fd..4f348e4 100644
> > --- a/platform/linux-generic/include/api/odp_buffer.h
> > +++ b/platform/linux-generic/include/api/odp_buffer.h
> > @@ -21,7 +21,10 @@ extern "C" {
> >
> >  #include <odp_std_types.h>
> >
> > -
> > +/**
> > + * @addtogroup buffer
> > + * @{
> > + */
> >
> >  /**
> >   * ODP buffer
> > @@ -83,6 +86,7 @@ int odp_buffer_is_valid(odp_buffer_t buf);
> >   */
> >  void odp_buffer_print(odp_buffer_t buf);
> >
> > +/** @} */
> >
> >  #ifdef __cplusplus
> >  }
> > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h
> b/platform/linux-generic/include/api/odp_buffer_pool.h
> > index fe88898..dd0ac43 100644
> > --- a/platform/linux-generic/include/api/odp_buffer_pool.h
> > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h
> > @@ -23,6 +23,11 @@ extern "C" {
> >  #include <odp_std_types.h>
> >  #include <odp_buffer.h>
> >
> > +/**
> > + * @addtogroup buffer ODP Buffers and buffer Pools
> > + * @{
> > + */
> > +
> >  /** Maximum queue name lenght in chars */
> >  #define ODP_BUFFER_POOL_NAME_LEN  32
> >
> > @@ -99,6 +104,7 @@ void odp_buffer_free(odp_buffer_t buf);
> >   */
> >  odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
> >
> > +/** @} */
> >
> >  #ifdef __cplusplus
> >  }
> > --
> > 1.9.1
> >
>
>
>
Mike Holmes Sept. 4, 2014, 3:11 p.m. UTC | #5
On 4 September 2014 11:01, Stuart Haslam <stuart.haslam@arm.com> wrote:

> On Thu, Sep 04, 2014 at 03:41:17PM +0100, Victor Kamensky wrote:
> > On 4 September 2014 03:10, Stuart Haslam <stuart.haslam@arm.com> wrote:
> > > On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> > >> ---
> > >>
> > >> This creates a new section in the documentaion to group everything
> > >> related to buffers. It appears to make things much more acessable
> > >> although it needs to have some real description added to the section.
> > >>
> > >
> > > Looks better to me. It also allows more freedom within the
> implementation
> > > to place things in different header files and still have the
> documentation
> > > look the same.
> >
> > If implementations will have freedom to place things in different
> headers,
> > how one app source will work with all of them? Which header it would
> > include? I am not against fancy doxygen syntax, but mapping ODP
> > symbol to header file name should be normative part of ODP. Of course,
> > such mapping could be supported with set of implementation defined
> > headers that are internally included by one that constitute ODP API.
> >
> > Thanks,
> > Victor
> >
>
> This has come up before and I think the intention has always been that
> the application should just include odp.h and the implementation can do
> whatever it likes with header files beyond that (except perhaps that
> they need to be named odp_*.h).
>
> Personally though, I would rather the file names were fixed, and I did
> wonder when writing that comment whether this decision should be
> revisited.
>
> I think in practice this will all work out nicely, but we need to work
with it a little, so we can work out the kinks.

> --
> Stuart.
>
>
vkamensky Sept. 4, 2014, 3:54 p.m. UTC | #6
On 4 September 2014 08:11, Mike Holmes <mike.holmes@linaro.org> wrote:
>
>
>
> On 4 September 2014 11:01, Stuart Haslam <stuart.haslam@arm.com> wrote:
>>
>> On Thu, Sep 04, 2014 at 03:41:17PM +0100, Victor Kamensky wrote:
>> > On 4 September 2014 03:10, Stuart Haslam <stuart.haslam@arm.com> wrote:
>> > > On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
>> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>> > >> ---
>> > >>
>> > >> This creates a new section in the documentaion to group everything
>> > >> related to buffers. It appears to make things much more acessable
>> > >> although it needs to have some real description added to the section.
>> > >>
>> > >
>> > > Looks better to me. It also allows more freedom within the
>> > > implementation
>> > > to place things in different header files and still have the
>> > > documentation
>> > > look the same.
>> >
>> > If implementations will have freedom to place things in different
>> > headers,
>> > how one app source will work with all of them? Which header it would
>> > include? I am not against fancy doxygen syntax, but mapping ODP
>> > symbol to header file name should be normative part of ODP. Of course,
>> > such mapping could be supported with set of implementation defined
>> > headers that are internally included by one that constitute ODP API.
>> >
>> > Thanks,
>> > Victor
>> >
>>
>> This has come up before and I think the intention has always been that
>> the application should just include odp.h and the implementation can do
>> whatever it likes with header files beyond that (except perhaps that
>> they need to be named odp_*.h).
>>
>> Personally though, I would rather the file names were fixed, and I did
>> wonder when writing that comment whether this decision should be
>> revisited.
>>
> I think in practice this will all work out nicely, but we need to work with
> it a little, so we can work out the kinks.

Not sure which way you argue :). Wrt of Stuart's only one odp.h few
remarks:

o in linux-generic odp_crypto.h and odp_rwlock.h are not included by
odp.h. I guess it is not only me who did not know about one odp.h file rule.
I think it should be fixed (i.e odp_crypto.h and odp_rwlock.h) must be
added to odp.h.

o Examples usage look consistent: most of examples include only odp.h
and helper header files. Only place I found is examples/generator includes
odp_packet_io.h in addition to odp.h (but it should not, because odp.h
does include odp_packet_io.h already)

o ODP API Design Guidelines
https://www.google.com/url?q=https%3A%2F%2Fdocs.google.com%2Fa%2Flinaro.org%2Fdocument%2Fd%2F15ltgSZolCeN66Xmx9rBSAzpxujia0wj4iPrqb2yzlb8%2Fedit
needs section that describes one odp.h header convention, if it is
not there yet (I could not find).

Thanks,
Victor

>>
>> --
>> Stuart.
>>
>
>
>
> --
> Mike Holmes
> Linaro Technical Manager / Lead
> LNG - ODP
Mike Holmes Sept. 4, 2014, 4:05 p.m. UTC | #7
On 4 September 2014 11:54, Victor Kamensky <victor.kamensky@linaro.org>
wrote:

> On 4 September 2014 08:11, Mike Holmes <mike.holmes@linaro.org> wrote:
> >
> >
> >
> > On 4 September 2014 11:01, Stuart Haslam <stuart.haslam@arm.com> wrote:
> >>
> >> On Thu, Sep 04, 2014 at 03:41:17PM +0100, Victor Kamensky wrote:
> >> > On 4 September 2014 03:10, Stuart Haslam <stuart.haslam@arm.com>
> wrote:
> >> > > On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
> >> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> >> > >> ---
> >> > >>
> >> > >> This creates a new section in the documentaion to group everything
> >> > >> related to buffers. It appears to make things much more acessable
> >> > >> although it needs to have some real description added to the
> section.
> >> > >>
> >> > >
> >> > > Looks better to me. It also allows more freedom within the
> >> > > implementation
> >> > > to place things in different header files and still have the
> >> > > documentation
> >> > > look the same.
> >> >
> >> > If implementations will have freedom to place things in different
> >> > headers,
> >> > how one app source will work with all of them? Which header it would
> >> > include? I am not against fancy doxygen syntax, but mapping ODP
> >> > symbol to header file name should be normative part of ODP. Of course,
> >> > such mapping could be supported with set of implementation defined
> >> > headers that are internally included by one that constitute ODP API.
> >> >
> >> > Thanks,
> >> > Victor
> >> >
> >>
> >> This has come up before and I think the intention has always been that
> >> the application should just include odp.h and the implementation can do
> >> whatever it likes with header files beyond that (except perhaps that
> >> they need to be named odp_*.h).
> >>
> >> Personally though, I would rather the file names were fixed, and I did
> >> wonder when writing that comment whether this decision should be
> >> revisited.
> >>
> > I think in practice this will all work out nicely, but we need to work
> with
> > it a little, so we can work out the kinks.
>
> Not sure which way you argue :). Wrt of Stuart's only one odp.h few
> remarks:
>
> o in linux-generic odp_crypto.h and odp_rwlock.h are not included by
> odp.h. I guess it is not only me who did not know about one odp.h file
> rule.
> I think it should be fixed (i.e odp_crypto.h and odp_rwlock.h) must be
> added to odp.h.
>
> Agree, I had to do this in my global_init fix, but it should just be done.
Personally I don't believe in odp.h, I think you should include only what
you need.


> o Examples usage look consistent: most of examples include only odp.h
> and helper header files. Only place I found is examples/generator includes
> odp_packet_io.h in addition to odp.h (but it should not, because odp.h
> does include odp_packet_io.h already)
>

Agree

>
> o ODP API Design Guidelines
>
> https://www.google.com/url?q=https%3A%2F%2Fdocs.google.com%2Fa%2Flinaro.org%2Fdocument%2Fd%2F15ltgSZolCeN66Xmx9rBSAzpxujia0wj4iPrqb2yzlb8%2Fedit
> needs section that describes one odp.h header convention, if it is
> not there yet (I could not find).
>

Agree

>
> Thanks,
> Victor
>
> >>
> >> --
> >> Stuart.
> >>
> >
> >
> >
> > --
> > Mike Holmes
> > Linaro Technical Manager / Lead
> > LNG - ODP
>
Taras Kondratiuk Sept. 4, 2014, 4:11 p.m. UTC | #8
On 09/04/2014 07:05 PM, Mike Holmes wrote:
>
>
>
> On 4 September 2014 11:54, Victor Kamensky <victor.kamensky@linaro.org
> <mailto:victor.kamensky@linaro.org>> wrote:
>
>     On 4 September 2014 08:11, Mike Holmes <mike.holmes@linaro.org
>     <mailto:mike.holmes@linaro.org>> wrote:
>      >
>      >
>      >
>      > On 4 September 2014 11:01, Stuart Haslam <stuart.haslam@arm.com
>     <mailto:stuart.haslam@arm.com>> wrote:
>      >>
>      >> On Thu, Sep 04, 2014 at 03:41:17PM +0100, Victor Kamensky wrote:
>      >> > On 4 September 2014 03:10, Stuart Haslam
>     <stuart.haslam@arm.com <mailto:stuart.haslam@arm.com>> wrote:
>      >> > > On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
>      >> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>     <mailto:mike.holmes@linaro.org>>
>      >> > >> ---
>      >> > >>
>      >> > >> This creates a new section in the documentaion to group
>     everything
>      >> > >> related to buffers. It appears to make things much more
>     acessable
>      >> > >> although it needs to have some real description added to
>     the section.
>      >> > >>
>      >> > >
>      >> > > Looks better to me. It also allows more freedom within the
>      >> > > implementation
>      >> > > to place things in different header files and still have the
>      >> > > documentation
>      >> > > look the same.
>      >> >
>      >> > If implementations will have freedom to place things in different
>      >> > headers,
>      >> > how one app source will work with all of them? Which header it
>     would
>      >> > include? I am not against fancy doxygen syntax, but mapping ODP
>      >> > symbol to header file name should be normative part of ODP. Of
>     course,
>      >> > such mapping could be supported with set of implementation defined
>      >> > headers that are internally included by one that constitute
>     ODP API.
>      >> >
>      >> > Thanks,
>      >> > Victor
>      >> >
>      >>
>      >> This has come up before and I think the intention has always
>     been that
>      >> the application should just include odp.h and the implementation
>     can do
>      >> whatever it likes with header files beyond that (except perhaps that
>      >> they need to be named odp_*.h).
>      >>
>      >> Personally though, I would rather the file names were fixed, and
>     I did
>      >> wonder when writing that comment whether this decision should be
>      >> revisited.
>      >>
>      > I think in practice this will all work out nicely, but we need to
>     work with
>      > it a little, so we can work out the kinks.
>
>     Not sure which way you argue :). Wrt of Stuart's only one odp.h few
>     remarks:
>
>     o in linux-generic odp_crypto.h and odp_rwlock.h are not included by
>     odp.h. I guess it is not only me who did not know about one odp.h
>     file rule.
>     I think it should be fixed (i.e odp_crypto.h and odp_rwlock.h) must be
>     added to odp.h.
>
> Agree, I had to do this in my global_init fix, but it should just be
> done. Personally I don't believe in odp.h, I think you should include
> only what you need.

Mike, which disadvantages common odp.h has?
Was there some mail thread there pros and cons of common odp.h were
discussed.
Mike Holmes Sept. 4, 2014, 4:21 p.m. UTC | #9
I think it obfuscates things, adds overhead to maintainence and to app
development.

It has to be maintained but it is not strictly necessary, as now, it is
already out of date and no one noticed
Implementers accidently use it rather than the internal file - overhead to
police this.
Applications no longer advertise the features they use in their include
list, I like that summary at the top of a file, it clearly says that this
file deals with crypto for example.
When hunting for information I have to go from my app.c to odp.h to find
the name of the real include file wanted to look in.
I don't like the conceptual coupling, for me coupling is bad when it is not
needed for execution - as in this case.

The only upside is that apps writers don't wear out their keyboards



On 4 September 2014 12:11, Taras Kondratiuk <taras.kondratiuk@linaro.org>
wrote:

> On 09/04/2014 07:05 PM, Mike Holmes wrote:
>
>>
>>
>>
>> On 4 September 2014 11:54, Victor Kamensky <victor.kamensky@linaro.org
>> <mailto:victor.kamensky@linaro.org>> wrote:
>>
>>     On 4 September 2014 08:11, Mike Holmes <mike.holmes@linaro.org
>>     <mailto:mike.holmes@linaro.org>> wrote:
>>      >
>>      >
>>      >
>>      > On 4 September 2014 11:01, Stuart Haslam <stuart.haslam@arm.com
>>     <mailto:stuart.haslam@arm.com>> wrote:
>>      >>
>>      >> On Thu, Sep 04, 2014 at 03:41:17PM +0100, Victor Kamensky wrote:
>>      >> > On 4 September 2014 03:10, Stuart Haslam
>>     <stuart.haslam@arm.com <mailto:stuart.haslam@arm.com>> wrote:
>>      >> > > On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
>>      >> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org
>>     <mailto:mike.holmes@linaro.org>>
>>
>>      >> > >> ---
>>      >> > >>
>>      >> > >> This creates a new section in the documentaion to group
>>     everything
>>      >> > >> related to buffers. It appears to make things much more
>>     acessable
>>      >> > >> although it needs to have some real description added to
>>     the section.
>>      >> > >>
>>      >> > >
>>      >> > > Looks better to me. It also allows more freedom within the
>>      >> > > implementation
>>      >> > > to place things in different header files and still have the
>>      >> > > documentation
>>      >> > > look the same.
>>      >> >
>>      >> > If implementations will have freedom to place things in
>> different
>>      >> > headers,
>>      >> > how one app source will work with all of them? Which header it
>>     would
>>      >> > include? I am not against fancy doxygen syntax, but mapping ODP
>>      >> > symbol to header file name should be normative part of ODP. Of
>>     course,
>>      >> > such mapping could be supported with set of implementation
>> defined
>>      >> > headers that are internally included by one that constitute
>>     ODP API.
>>      >> >
>>      >> > Thanks,
>>      >> > Victor
>>      >> >
>>      >>
>>      >> This has come up before and I think the intention has always
>>     been that
>>      >> the application should just include odp.h and the implementation
>>     can do
>>      >> whatever it likes with header files beyond that (except perhaps
>> that
>>      >> they need to be named odp_*.h).
>>      >>
>>      >> Personally though, I would rather the file names were fixed, and
>>     I did
>>      >> wonder when writing that comment whether this decision should be
>>      >> revisited.
>>      >>
>>      > I think in practice this will all work out nicely, but we need to
>>     work with
>>      > it a little, so we can work out the kinks.
>>
>>     Not sure which way you argue :). Wrt of Stuart's only one odp.h few
>>     remarks:
>>
>>     o in linux-generic odp_crypto.h and odp_rwlock.h are not included by
>>     odp.h. I guess it is not only me who did not know about one odp.h
>>     file rule.
>>     I think it should be fixed (i.e odp_crypto.h and odp_rwlock.h) must be
>>     added to odp.h.
>>
>> Agree, I had to do this in my global_init fix, but it should just be
>> done. Personally I don't believe in odp.h, I think you should include
>> only what you need.
>>
>
> Mike, which disadvantages common odp.h has?
> Was there some mail thread there pros and cons of common odp.h were
> discussed.
>
Taras Kondratiuk Sept. 4, 2014, 4:56 p.m. UTC | #10
On 09/04/2014 07:21 PM, Mike Holmes wrote:
> I think it obfuscates things, adds overhead to maintainence and to app
> development.
>
> It has to be maintained but it is not strictly necessary, as now, it is
> already out of date and no one noticed
> Implementers accidently use it rather than the internal file - overhead
> to police this.
> Applications no longer advertise the features they use in their include
> list, I like that summary at the top of a file, it clearly says that
> this file deals with crypto for example.
> When hunting for information I have to go from my app.c to odp.h to find
> the name of the real include file wanted to look in.
> I don't like the conceptual coupling, for me coupling is bad when it is
> not needed for execution - as in this case.
>
> The only upside is that apps writers don't wear out their keyboards

I partially agree, but having separate headers exposed to application
makes header names a part of specification. So if let's say in future
we would like to reorganize headers and move all type definitions to a
separate file, then all applications have to be updated.
Mike Holmes Sept. 4, 2014, 5:16 p.m. UTC | #11
On 4 September 2014 12:56, Taras Kondratiuk <taras.kondratiuk@linaro.org>
wrote:

> On 09/04/2014 07:21 PM, Mike Holmes wrote:
>
>> I think it obfuscates things, adds overhead to maintainence and to app
>> development.
>>
>> It has to be maintained but it is not strictly necessary, as now, it is
>> already out of date and no one noticed
>> Implementers accidently use it rather than the internal file - overhead
>> to police this.
>> Applications no longer advertise the features they use in their include
>> list, I like that summary at the top of a file, it clearly says that
>> this file deals with crypto for example.
>> When hunting for information I have to go from my app.c to odp.h to find
>> the name of the real include file wanted to look in.
>> I don't like the conceptual coupling, for me coupling is bad when it is
>> not needed for execution - as in this case.
>>
>> The only upside is that apps writers don't wear out their keyboards
>>
>
> I partially agree, but having separate headers exposed to application
> makes header names a part of specification. So if let's say in future
> we would like to reorganize headers and move all type definitions to a
> separate file, then all applications have to be updated.
>

I would pay that price and call it ODP 3.0, you can't always ensure
backwards compatibility or you stagnate.
Mike Holmes Sept. 4, 2014, 9:34 p.m. UTC | #12
I experimented with defgroup and ingroup.

Using ingroup means you have to tag each thing individually, trying to
combine it with "@{"  was not successful so I think addtogroup is best, it
will concatenate large blocks of API.
In future we can go mark each thing with @ingroup <block name>  if we need
that atomicity
defgroup can replace the major section addtogroup without a problem.

So having one defgroup with other regions marked with addtogroup appears
the way forward to me.

Mike


On 4 September 2014 11:07, Mike Holmes <mike.holmes@linaro.org> wrote:

>
>
>
> On 4 September 2014 06:10, Stuart Haslam <stuart.haslam@arm.com> wrote:
>
>> On Wed, Sep 03, 2014 at 03:18:11AM +0100, Mike Holmes wrote:
>> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
>> > ---
>> >
>> > This creates a new section in the documentaion to group everything
>> > related to buffers. It appears to make things much more acessable
>> > although it needs to have some real description added to the section.
>> >
>>
>> Looks better to me. It also allows more freedom within the implementation
>> to place things in different header files and still have the documentation
>> look the same.
>>
>> Small nit, I think it would be better to use defgroup and ingroup, rather
>> than addtogroup, to avoid any confusion about where the documentation for
>> that group should go (what happens if you document multiple addtogroups?).
>>
>
>
> We can make it explicit, I was thinking that it was easier to manage with
> the looser association.
> multiple add groups just concatenate without a warning.
>
>
>
>
>> --
>> Stuart.
>>
>> >  platform/linux-generic/include/api/odp_buffer.h      | 6 +++++-
>> >  platform/linux-generic/include/api/odp_buffer_pool.h | 6 ++++++
>> >  2 files changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/platform/linux-generic/include/api/odp_buffer.h
>> b/platform/linux-generic/include/api/odp_buffer.h
>> > index d8577fd..4f348e4 100644
>> > --- a/platform/linux-generic/include/api/odp_buffer.h
>> > +++ b/platform/linux-generic/include/api/odp_buffer.h
>> > @@ -21,7 +21,10 @@ extern "C" {
>> >
>> >  #include <odp_std_types.h>
>> >
>> > -
>> > +/**
>> > + * @addtogroup buffer
>> > + * @{
>> > + */
>> >
>> >  /**
>> >   * ODP buffer
>> > @@ -83,6 +86,7 @@ int odp_buffer_is_valid(odp_buffer_t buf);
>> >   */
>> >  void odp_buffer_print(odp_buffer_t buf);
>> >
>> > +/** @} */
>> >
>> >  #ifdef __cplusplus
>> >  }
>> > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h
>> b/platform/linux-generic/include/api/odp_buffer_pool.h
>> > index fe88898..dd0ac43 100644
>> > --- a/platform/linux-generic/include/api/odp_buffer_pool.h
>> > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h
>> > @@ -23,6 +23,11 @@ extern "C" {
>> >  #include <odp_std_types.h>
>> >  #include <odp_buffer.h>
>> >
>> > +/**
>> > + * @addtogroup buffer ODP Buffers and buffer Pools
>> > + * @{
>> > + */
>> > +
>> >  /** Maximum queue name lenght in chars */
>> >  #define ODP_BUFFER_POOL_NAME_LEN  32
>> >
>> > @@ -99,6 +104,7 @@ void odp_buffer_free(odp_buffer_t buf);
>> >   */
>> >  odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
>> >
>> > +/** @} */
>> >
>> >  #ifdef __cplusplus
>> >  }
>> > --
>> > 1.9.1
>> >
>>
>>
>>
>
>
> --
> *Mike Holmes*
> Linaro Technical Manager / Lead
> LNG - ODP
>
Stuart Haslam Sept. 4, 2014, 10:25 p.m. UTC | #13
On Thu, Sep 04, 2014 at 10:34:24PM +0100, Mike Holmes wrote:
> I experimented with defgroup and ingroup.
> 
> Using ingroup means you have to tag each thing individually, trying to combine it with "@{"  was not successful so I think addtogroup is best, it will concatenate large blocks of API.
> In future we can go mark each thing with @ingroup <block name>  if we need that atomicity
> defgroup can replace the major section addtogroup without a problem.
> 
> So having one defgroup with other regions marked with addtogroup appears the way forward to me.
> 

I agree, I didn't know about that restriction with ingroup.

--
Stuart.
Taras Kondratiuk Sept. 5, 2014, 8:13 a.m. UTC | #14
On 09/04/2014 08:16 PM, Mike Holmes wrote:
>
>
>
> On 4 September 2014 12:56, Taras Kondratiuk <taras.kondratiuk@linaro.org
> <mailto:taras.kondratiuk@linaro.org>> wrote:
>
>     On 09/04/2014 07:21 PM, Mike Holmes wrote:
>
>         I think it obfuscates things, adds overhead to maintainence and
>         to app
>         development.
>
>         It has to be maintained but it is not strictly necessary, as
>         now, it is
>         already out of date and no one noticed
>         Implementers accidently use it rather than the internal file -
>         overhead
>         to police this.
>         Applications no longer advertise the features they use in their
>         include
>         list, I like that summary at the top of a file, it clearly says that
>         this file deals with crypto for example.
>         When hunting for information I have to go from my app.c to odp.h
>         to find
>         the name of the real include file wanted to look in.
>         I don't like the conceptual coupling, for me coupling is bad
>         when it is
>         not needed for execution - as in this case.
>
>         The only upside is that apps writers don't wear out their keyboards
>
>
>     I partially agree, but having separate headers exposed to application
>     makes header names a part of specification. So if let's say in future
>     we would like to reorganize headers and move all type definitions to a
>     separate file, then all applications have to be updated.
>
>
> I would pay that price and call it ODP 3.0, you can't always ensure
> backwards compatibility or you stagnate.

Ok, then IMO before ODP v1.0 we have to revise header names and its
content. Decide which of them are public and add this to specification.
IMO currently we have too many public headers. For example is
odp_packet_flags.h need to be included by application or it can be 
implicitly always included by odp_packet.h?
diff mbox

Patch

diff --git a/platform/linux-generic/include/api/odp_buffer.h b/platform/linux-generic/include/api/odp_buffer.h
index d8577fd..4f348e4 100644
--- a/platform/linux-generic/include/api/odp_buffer.h
+++ b/platform/linux-generic/include/api/odp_buffer.h
@@ -21,7 +21,10 @@  extern "C" {
 
 #include <odp_std_types.h>
 
-
+/**
+ * @addtogroup buffer
+ * @{
+ */
 
 /**
  * ODP buffer
@@ -83,6 +86,7 @@  int odp_buffer_is_valid(odp_buffer_t buf);
  */
 void odp_buffer_print(odp_buffer_t buf);
 
+/** @} */
 
 #ifdef __cplusplus
 }
diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h
index fe88898..dd0ac43 100644
--- a/platform/linux-generic/include/api/odp_buffer_pool.h
+++ b/platform/linux-generic/include/api/odp_buffer_pool.h
@@ -23,6 +23,11 @@  extern "C" {
 #include <odp_std_types.h>
 #include <odp_buffer.h>
 
+/**
+ * @addtogroup buffer ODP Buffers and buffer Pools
+ * @{
+ */
+
 /** Maximum queue name lenght in chars */
 #define ODP_BUFFER_POOL_NAME_LEN  32
 
@@ -99,6 +104,7 @@  void odp_buffer_free(odp_buffer_t buf);
  */
 odp_buffer_pool_t odp_buffer_pool(odp_buffer_t buf);
 
+/** @} */
 
 #ifdef __cplusplus
 }