[PATCHv2] api: fix odp_version_api_str()

Message ID 20150203224842.GL10838@8074w.roxell.se
State New
Headers show

Commit Message

Anders Roxell Feb. 3, 2015, 10:48 p.m.
On 2015-02-03 11:28, Mike Holmes wrote:
> On 3 February 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> 
> > odp_version_api_str() has to be in API header while
> > odp_version_impl_str() should be in linux-generic.
> > That change fixes:
> > https://bugs.linaro.org/show_bug.cgi?id=1194
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> >
> 
> Reviewed-by: Mike Holmes <mike.holmes@linaro.org>
> 
> Clears the new test case patch "validation: add version string check"
> which should be added as insurance against this bug in the future before
> closing the bug

I think we can drop "static inline" to the version string function,
because checking for a version string is not time critical right?

If we assume that its not time critical the diff below + Mikes validation
version string patch got it to pass and we hide more stuff from the
public doxygen clean API include/odp/api/*.h files.

Cheers,
Anders

$ git df

Comments

Maxim Uvarov Feb. 4, 2015, 8:45 a.m. | #1
odp_version_api_str() is API function or not? If it's api it has to be in api header. If it's implementation only helper it hat to be under linux-generic.

Maxim.



On 02/04/2015 01:48 AM, Anders Roxell wrote:
> On 2015-02-03 11:28, Mike Holmes wrote:
>> On 3 February 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>>
>>> odp_version_api_str() has to be in API header while
>>> odp_version_impl_str() should be in linux-generic.
>>> That change fixes:
>>> https://bugs.linaro.org/show_bug.cgi?id=1194
>>>
>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>
>> Reviewed-by: Mike Holmes <mike.holmes@linaro.org>
>>
>> Clears the new test case patch "validation: add version string check"
>> which should be added as insurance against this bug in the future before
>> closing the bug
> I think we can drop "static inline" to the version string function,
> because checking for a version string is not time critical right?
>
> If we assume that its not time critical the diff below + Mikes validation
> version string patch got it to pass and we hide more stuff from the
> public doxygen clean API include/odp/api/*.h files.
>
> Cheers,
> Anders
>
> $ git df
> diff --git a/platform/linux-generic/include/odp/version.h b/platform/linux-generic/include/odp/version.h
> index f29320a..2c557cc 100644
> --- a/platform/linux-generic/include/odp/version.h
> +++ b/platform/linux-generic/include/odp/version.h
> @@ -23,11 +23,6 @@ extern "C" {
>    *  @{
>    */
>   
> -static inline const char *odp_version_api_str(void)
> -{
> -       return ODP_VERSION_API_STR;
> -}
> -
>   /**
>    * @}
>    */
> diff --git a/platform/linux-generic/odp_impl.c b/platform/linux-generic/odp_impl.c
> index ca3224d..23ab12d 100644
> --- a/platform/linux-generic/odp_impl.c
> +++ b/platform/linux-generic/odp_impl.c
> @@ -28,6 +28,11 @@ const char *odp_version_impl_str(void)
>          return ODP_VERSION_IMPL_STR;
>   }
>   
> +const char *odp_version_api_str(void)
> +{
> +       return ODP_VERSION_API_STR;
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif
Maxim Uvarov Feb. 4, 2015, 9:12 a.m. | #2
Regarding to "static inline" even if it's not performance critical I 
think it's reasonable to have one line function in single header.

Or even switch it to:
#define odp_version_api_str() ODP_VERSION_API_STR

Maxim.

On 02/04/2015 11:45 AM, Maxim Uvarov wrote:
> odp_version_api_str() is API function or not? If it's api it has to be 
> in api header. If it's implementation only helper it hat to be under 
> linux-generic.
>
> Maxim.
>
>
>
> On 02/04/2015 01:48 AM, Anders Roxell wrote:
>> On 2015-02-03 11:28, Mike Holmes wrote:
>>> On 3 February 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org> 
>>> wrote:
>>>
>>>> odp_version_api_str() has to be in API header while
>>>> odp_version_impl_str() should be in linux-generic.
>>>> That change fixes:
>>>> https://bugs.linaro.org/show_bug.cgi?id=1194
>>>>
>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>>
>>> Reviewed-by: Mike Holmes <mike.holmes@linaro.org>
>>>
>>> Clears the new test case patch "validation: add version string check"
>>> which should be added as insurance against this bug in the future 
>>> before
>>> closing the bug
>> I think we can drop "static inline" to the version string function,
>> because checking for a version string is not time critical right?
>>
>> If we assume that its not time critical the diff below + Mikes 
>> validation
>> version string patch got it to pass and we hide more stuff from the
>> public doxygen clean API include/odp/api/*.h files.
>>
>> Cheers,
>> Anders
>>
>> $ git df
>> diff --git a/platform/linux-generic/include/odp/version.h 
>> b/platform/linux-generic/include/odp/version.h
>> index f29320a..2c557cc 100644
>> --- a/platform/linux-generic/include/odp/version.h
>> +++ b/platform/linux-generic/include/odp/version.h
>> @@ -23,11 +23,6 @@ extern "C" {
>>    *  @{
>>    */
>>   -static inline const char *odp_version_api_str(void)
>> -{
>> -       return ODP_VERSION_API_STR;
>> -}
>> -
>>   /**
>>    * @}
>>    */
>> diff --git a/platform/linux-generic/odp_impl.c 
>> b/platform/linux-generic/odp_impl.c
>> index ca3224d..23ab12d 100644
>> --- a/platform/linux-generic/odp_impl.c
>> +++ b/platform/linux-generic/odp_impl.c
>> @@ -28,6 +28,11 @@ const char *odp_version_impl_str(void)
>>          return ODP_VERSION_IMPL_STR;
>>   }
>>   +const char *odp_version_api_str(void)
>> +{
>> +       return ODP_VERSION_API_STR;
>> +}
>> +
>>   #ifdef __cplusplus
>>   }
>>   #endif
>
Ola Liljedahl Feb. 4, 2015, 10:04 a.m. | #3
On 4 February 2015 at 10:12, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> Regarding to "static inline" even if it's not performance critical I think
> it's reasonable to have one line function in single header.
>
> Or even switch it to:
> #define odp_version_api_str() ODP_VERSION_API_STR
Make it a proper function (does not have to be inlined) and hide the
define from the application.
I want to avoid all explicit "inlining" of platform specific
definitions. We need to keep the door open for an ODP ABI and e.g.
shared library implementations.

>
> Maxim.
>
>
> On 02/04/2015 11:45 AM, Maxim Uvarov wrote:
>>
>> odp_version_api_str() is API function or not? If it's api it has to be in
>> api header. If it's implementation only helper it hat to be under
>> linux-generic.
>>
>> Maxim.
>>
>>
>>
>> On 02/04/2015 01:48 AM, Anders Roxell wrote:
>>>
>>> On 2015-02-03 11:28, Mike Holmes wrote:
>>>>
>>>> On 3 February 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org>
>>>> wrote:
>>>>
>>>>> odp_version_api_str() has to be in API header while
>>>>> odp_version_impl_str() should be in linux-generic.
>>>>> That change fixes:
>>>>> https://bugs.linaro.org/show_bug.cgi?id=1194
>>>>>
>>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>>>
>>>> Reviewed-by: Mike Holmes <mike.holmes@linaro.org>
>>>>
>>>> Clears the new test case patch "validation: add version string check"
>>>> which should be added as insurance against this bug in the future before
>>>> closing the bug
>>>
>>> I think we can drop "static inline" to the version string function,
>>> because checking for a version string is not time critical right?
>>>
>>> If we assume that its not time critical the diff below + Mikes validation
>>> version string patch got it to pass and we hide more stuff from the
>>> public doxygen clean API include/odp/api/*.h files.
>>>
>>> Cheers,
>>> Anders
>>>
>>> $ git df
>>> diff --git a/platform/linux-generic/include/odp/version.h
>>> b/platform/linux-generic/include/odp/version.h
>>> index f29320a..2c557cc 100644
>>> --- a/platform/linux-generic/include/odp/version.h
>>> +++ b/platform/linux-generic/include/odp/version.h
>>> @@ -23,11 +23,6 @@ extern "C" {
>>>    *  @{
>>>    */
>>>   -static inline const char *odp_version_api_str(void)
>>> -{
>>> -       return ODP_VERSION_API_STR;
>>> -}
>>> -
>>>   /**
>>>    * @}
>>>    */
>>> diff --git a/platform/linux-generic/odp_impl.c
>>> b/platform/linux-generic/odp_impl.c
>>> index ca3224d..23ab12d 100644
>>> --- a/platform/linux-generic/odp_impl.c
>>> +++ b/platform/linux-generic/odp_impl.c
>>> @@ -28,6 +28,11 @@ const char *odp_version_impl_str(void)
>>>          return ODP_VERSION_IMPL_STR;
>>>   }
>>>   +const char *odp_version_api_str(void)
>>> +{
>>> +       return ODP_VERSION_API_STR;
>>> +}
>>> +
>>>   #ifdef __cplusplus
>>>   }
>>>   #endif
>>
>>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Mike Holmes Feb. 4, 2015, 2:43 p.m. | #4
If the goal is to fix the bug, Anders tiny patch does that, what else are
we fixing ?

I did not look deeply enough before because I did not understand the new
header layout properly, but both functions odp_version_api_str and
odp_version_impl_str are part of the public API and all implementations
need to implement them so they should be in include/odp/api/version.h I
think.




On 4 February 2015 at 05:04, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> On 4 February 2015 at 10:12, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> > Regarding to "static inline" even if it's not performance critical I
> think
> > it's reasonable to have one line function in single header.
> >
> > Or even switch it to:
> > #define odp_version_api_str() ODP_VERSION_API_STR
> Make it a proper function (does not have to be inlined) and hide the
> define from the application.
> I want to avoid all explicit "inlining" of platform specific
> definitions. We need to keep the door open for an ODP ABI and e.g.
> shared library implementations.
>
> >
> > Maxim.
> >
> >
> > On 02/04/2015 11:45 AM, Maxim Uvarov wrote:
> >>
> >> odp_version_api_str() is API function or not? If it's api it has to be
> in
> >> api header. If it's implementation only helper it hat to be under
> >> linux-generic.
> >>
> >> Maxim.
> >>
> >>
> >>
> >> On 02/04/2015 01:48 AM, Anders Roxell wrote:
> >>>
> >>> On 2015-02-03 11:28, Mike Holmes wrote:
> >>>>
> >>>> On 3 February 2015 at 11:12, Maxim Uvarov <maxim.uvarov@linaro.org>
> >>>> wrote:
> >>>>
> >>>>> odp_version_api_str() has to be in API header while
> >>>>> odp_version_impl_str() should be in linux-generic.
> >>>>> That change fixes:
> >>>>> https://bugs.linaro.org/show_bug.cgi?id=1194
> >>>>>
> >>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> >>>>>
> >>>> Reviewed-by: Mike Holmes <mike.holmes@linaro.org>
> >>>>
> >>>> Clears the new test case patch "validation: add version string check"
> >>>> which should be added as insurance against this bug in the future
> before
> >>>> closing the bug
> >>>
> >>> I think we can drop "static inline" to the version string function,
> >>> because checking for a version string is not time critical right?
> >>>
> >>> If we assume that its not time critical the diff below + Mikes
> validation
> >>> version string patch got it to pass and we hide more stuff from the
> >>> public doxygen clean API include/odp/api/*.h files.
> >>>
> >>> Cheers,
> >>> Anders
> >>>
> >>> $ git df
> >>> diff --git a/platform/linux-generic/include/odp/version.h
> >>> b/platform/linux-generic/include/odp/version.h
> >>> index f29320a..2c557cc 100644
> >>> --- a/platform/linux-generic/include/odp/version.h
> >>> +++ b/platform/linux-generic/include/odp/version.h
> >>> @@ -23,11 +23,6 @@ extern "C" {
> >>>    *  @{
> >>>    */
> >>>   -static inline const char *odp_version_api_str(void)
> >>> -{
> >>> -       return ODP_VERSION_API_STR;
> >>> -}
> >>> -
> >>>   /**
> >>>    * @}
> >>>    */
> >>> diff --git a/platform/linux-generic/odp_impl.c
> >>> b/platform/linux-generic/odp_impl.c
> >>> index ca3224d..23ab12d 100644
> >>> --- a/platform/linux-generic/odp_impl.c
> >>> +++ b/platform/linux-generic/odp_impl.c
> >>> @@ -28,6 +28,11 @@ const char *odp_version_impl_str(void)
> >>>          return ODP_VERSION_IMPL_STR;
> >>>   }
> >>>   +const char *odp_version_api_str(void)
> >>> +{
> >>> +       return ODP_VERSION_API_STR;
> >>> +}
> >>> +
> >>>   #ifdef __cplusplus
> >>>   }
> >>>   #endif
> >>
> >>
> >
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov Feb. 4, 2015, 3:26 p.m. | #5
On 02/04/2015 05:43 PM, Mike Holmes wrote:
> If the goal is to fix the bug, Anders tiny patch does that, what else 
> are we fixing ?
>
it's better to take a look at v4 of my patch.

odp_impc. has defines copy pasted from header:

-#ifndef ODP_IMPL_H_
-#define ODP_IMPL_H_

+ implementation of odp_version_api_str() should be there and it should 
not be inline.
So v4 patch is for that.

Maxim.


> I did not look deeply enough before because I did not understand the 
> new header layout properly, but both functions odp_version_api_str and 
> odp_version_impl_str are part of the public API and all 
> implementations need to implement them so they should be 
> in include/odp/api/version.h I think.
>
>
>
>
> On 4 February 2015 at 05:04, Ola Liljedahl <ola.liljedahl@linaro.org 
> <mailto:ola.liljedahl@linaro.org>> wrote:
>
>     On 4 February 2015 at 10:12, Maxim Uvarov <maxim.uvarov@linaro.org
>     <mailto:maxim.uvarov@linaro.org>> wrote:
>     > Regarding to "static inline" even if it's not performance
>     critical I think
>     > it's reasonable to have one line function in single header.
>     >
>     > Or even switch it to:
>     > #define odp_version_api_str() ODP_VERSION_API_STR
>     Make it a proper function (does not have to be inlined) and hide the
>     define from the application.
>     I want to avoid all explicit "inlining" of platform specific
>     definitions. We need to keep the door open for an ODP ABI and e.g.
>     shared library implementations.
>
>     >
>     > Maxim.
>     >
>     >
>     > On 02/04/2015 11:45 AM, Maxim Uvarov wrote:
>     >>
>     >> odp_version_api_str() is API function or not? If it's api it
>     has to be in
>     >> api header. If it's implementation only helper it hat to be under
>     >> linux-generic.
>     >>
>     >> Maxim.
>     >>
>     >>
>     >>
>     >> On 02/04/2015 01:48 AM, Anders Roxell wrote:
>     >>>
>     >>> On 2015-02-03 11:28, Mike Holmes wrote:
>     >>>>
>     >>>> On 3 February 2015 at 11:12, Maxim Uvarov
>     <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>>
>     >>>> wrote:
>     >>>>
>     >>>>> odp_version_api_str() has to be in API header while
>     >>>>> odp_version_impl_str() should be in linux-generic.
>     >>>>> That change fixes:
>     >>>>> https://bugs.linaro.org/show_bug.cgi?id=1194
>     >>>>>
>     >>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>     <mailto:maxim.uvarov@linaro.org>>
>     >>>>>
>     >>>> Reviewed-by: Mike Holmes <mike.holmes@linaro.org
>     <mailto:mike.holmes@linaro.org>>
>     >>>>
>     >>>> Clears the new test case patch "validation: add version
>     string check"
>     >>>> which should be added as insurance against this bug in the
>     future before
>     >>>> closing the bug
>     >>>
>     >>> I think we can drop "static inline" to the version string
>     function,
>     >>> because checking for a version string is not time critical right?
>     >>>
>     >>> If we assume that its not time critical the diff below + Mikes
>     validation
>     >>> version string patch got it to pass and we hide more stuff
>     from the
>     >>> public doxygen clean API include/odp/api/*.h files.
>     >>>
>     >>> Cheers,
>     >>> Anders
>     >>>
>     >>> $ git df
>     >>> diff --git a/platform/linux-generic/include/odp/version.h
>     >>> b/platform/linux-generic/include/odp/version.h
>     >>> index f29320a..2c557cc 100644
>     >>> --- a/platform/linux-generic/include/odp/version.h
>     >>> +++ b/platform/linux-generic/include/odp/version.h
>     >>> @@ -23,11 +23,6 @@ extern "C" {
>     >>>    *  @{
>     >>>    */
>     >>>   -static inline const char *odp_version_api_str(void)
>     >>> -{
>     >>> -       return ODP_VERSION_API_STR;
>     >>> -}
>     >>> -
>     >>>   /**
>     >>>    * @}
>     >>>    */
>     >>> diff --git a/platform/linux-generic/odp_impl.c
>     >>> b/platform/linux-generic/odp_impl.c
>     >>> index ca3224d..23ab12d 100644
>     >>> --- a/platform/linux-generic/odp_impl.c
>     >>> +++ b/platform/linux-generic/odp_impl.c
>     >>> @@ -28,6 +28,11 @@ const char *odp_version_impl_str(void)
>     >>>          return ODP_VERSION_IMPL_STR;
>     >>>   }
>     >>>   +const char *odp_version_api_str(void)
>     >>> +{
>     >>> +       return ODP_VERSION_API_STR;
>     >>> +}
>     >>> +
>     >>>   #ifdef __cplusplus
>     >>>   }
>     >>>   #endif
>     >>
>     >>
>     >
>     >
>     > _______________________________________________
>     > lng-odp mailing list
>     > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     > http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
> -- 
> *Mike Holmes*
> Linaro  Sr Technical Manager
> LNG - ODP

Patch

diff --git a/platform/linux-generic/include/odp/version.h b/platform/linux-generic/include/odp/version.h
index f29320a..2c557cc 100644
--- a/platform/linux-generic/include/odp/version.h
+++ b/platform/linux-generic/include/odp/version.h
@@ -23,11 +23,6 @@  extern "C" {
  *  @{
  */
 
-static inline const char *odp_version_api_str(void)
-{
-       return ODP_VERSION_API_STR;
-}
-
 /**
  * @}
  */
diff --git a/platform/linux-generic/odp_impl.c b/platform/linux-generic/odp_impl.c
index ca3224d..23ab12d 100644
--- a/platform/linux-generic/odp_impl.c
+++ b/platform/linux-generic/odp_impl.c
@@ -28,6 +28,11 @@  const char *odp_version_impl_str(void)
        return ODP_VERSION_IMPL_STR;
 }
 
+const char *odp_version_api_str(void)
+{
+       return ODP_VERSION_API_STR;
+}
+
 #ifdef __cplusplus
 }
 #endif