| Message ID | 20150203224842.GL10838@8074w.roxell.se |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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 >
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
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