| Message ID | 1487859248-23273-2-git-send-email-petri.savolainen@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [API-NEXT,1/4] api: hints: remove ODP_DEPRECATED from API | expand |
On Thu, Feb 23, 2017 at 8:14 AM, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Added ODP_DEPRECATED macro which is visible through API and > is used to control if deprecated API definitions are enabled > or disabled. > > Deprecated definitions are first moved behind this macro and > then removed completely in a later API version. Deprecated APIs > are disabled by default. > > Added configuration option --enable-deprecated to control the > macro value. > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > configure.ac | 19 +++++++++-- > include/odp/api/spec/.gitignore | 1 + > include/odp/api/spec/deprecated.h.in | 38 ++++++++++++++++++++++ > include/odp_api.h | 1 + > platform/Makefile.inc | 1 + > platform/linux-generic/Makefile.am | 1 + > .../linux-generic/include/odp/api/deprecated.h | 26 +++++++++++++++ > 7 files changed, 85 insertions(+), 2 deletions(-) > create mode 100644 include/odp/api/spec/deprecated.h.in > create mode 100644 platform/linux-generic/include/odp/api/deprecated.h > > diff --git a/configure.ac b/configure.ac > index e4bd3a7..9927916 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -16,7 +16,8 @@ ODP_VERSION_API_MAJOR=odpapi_major_version > AC_SUBST(ODP_VERSION_API_MAJOR) > ODP_VERSION_API_MINOR=odpapi_minor_version > AC_SUBST(ODP_VERSION_API_MINOR) > -AC_CONFIG_FILES([include/odp/api/spec/version.h]) > +AC_CONFIG_FILES([include/odp/api/spec/version.h > + include/odp/api/spec/deprecated.h]) > > AM_INIT_AUTOMAKE([1.9 tar-pax subdir-objects]) > AC_CONFIG_SRCDIR([helper/config.h.in]) > @@ -284,7 +285,7 @@ ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG" > ODP_ABI_COMPAT=1 > abi_compat=yes > AC_ARG_ENABLE([abi-compat], > - [ --disable-abi-compat disables ABI compatible mode, enables inline code in header files], > + [ --disable-abi-compat disables ABI compatible mode, enables inline code in header files], > [if test "x$enableval" = "xno"; then > ODP_ABI_COMPAT=0 > abi_compat=no > @@ -294,6 +295,19 @@ AC_ARG_ENABLE([abi-compat], > AC_SUBST(ODP_ABI_COMPAT) > > ########################################################################## > +# Enable/disable deprecated API definitions > +########################################################################## > +ODP_DEPRECATED=0 > +deprecated=no > +AC_ARG_ENABLE([deprecated], > + [ --enable-deprecated enable deprecated API definitions], > + [if test "x$enableval" = "xyes"; then > + ODP_DEPRECATED=1 > + deprecated=yes > + fi]) > +AC_SUBST(ODP_DEPRECATED) > + > +########################################################################## > # Default warning setup > ########################################################################## > ODP_CFLAGS="$ODP_CFLAGS -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes" > @@ -379,6 +393,7 @@ AC_MSG_RESULT([ > static libraries: ${enable_static} > shared libraries: ${enable_shared} > ABI compatible: ${abi_compat} > + Deprecated APIs: ${deprecated} > cunit: ${cunit_support} > test_vald: ${test_vald} > test_perf: ${test_perf} > diff --git a/include/odp/api/spec/.gitignore b/include/odp/api/spec/.gitignore > index 6702033..df9c87d 100644 > --- a/include/odp/api/spec/.gitignore > +++ b/include/odp/api/spec/.gitignore > @@ -1 +1,2 @@ > +deprecated.h > version.h > diff --git a/include/odp/api/spec/deprecated.h.in b/include/odp/api/spec/deprecated.h.in > new file mode 100644 > index 0000000..a319064 > --- /dev/null > +++ b/include/odp/api/spec/deprecated.h.in > @@ -0,0 +1,38 @@ > +/* Copyright (c) 2017, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +/** > + * @file > + * > + * Macro for deprecated API definitions > + */ > + > +#ifndef ODP_API_DEPRECATED_H_ > +#define ODP_API_DEPRECATED_H_ > +#include <odp/visibility_begin.h> > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +/** > + * Deprecated API definitions > + * > + * Some API definitions may be deprecated by this or a previous API version. > + * This macro controls if those are enabled (and visible to the application) > + * or disabled. > + * > + * * 0: Deprecated API definitions are disabled > + * * 1: Deprecated API definitions are enabled > + */ > +#define ODP_DEPRECATED @ODP_DEPRECATED@ I am confused here because this turns a compiler dependency into a build system + compiler dependency. Lots of libraries choose to use Autotools as a build system, but it's unnatural to require API header files like odp.h to also use Autotools. Anyone can grab these API header files and: include them in their application to use ODP, to create stubs for an ODP implementation, and most importantly they should be usable with any build system (Makefile, Autotools, CMake, ninja...). The original problem stated here is that the current code for the deprecated macro requires the use of GCC. The fix, or the way to address portability, can simply be to conditionally compile different code for different compilers: #ifdef __GCC__ #define DEPRECATED __attribute__((deprecated)) #elif MSCV #define DEPRECATED __declspec(deprecated) #else // define it to nothing... a nop... #define DEPRECATED // or... if an implementation must be required for all compilers: #error Please add support for your compiler in compiler.h #endif It is succinct an easy to maintain compared to embedding these portability concerns deeper into areas such as the build system and API header files. The same approach to portability can be used for architecturally specific C code as well as any other system specific macros provided by the compiler. > +#ifdef __cplusplus > +} > +#endif > + > +#include <odp/visibility_end.h> > +#endif > diff --git a/include/odp_api.h b/include/odp_api.h > index 73e5309..962415f 100644 > --- a/include/odp_api.h > +++ b/include/odp_api.h > @@ -18,6 +18,7 @@ > extern "C" { > #endif > > +#include <odp/api/deprecated.h> > #include <odp/api/version.h> > #include <odp/api/std_types.h> > #include <odp/api/compiler.h> > diff --git a/platform/Makefile.inc b/platform/Makefile.inc > index 874cf88..f282770 100644 > --- a/platform/Makefile.inc > +++ b/platform/Makefile.inc > @@ -29,6 +29,7 @@ odpapispecinclude_HEADERS = \ > $(top_srcdir)/include/odp/api/spec/cpumask.h \ > $(top_srcdir)/include/odp/api/spec/crypto.h \ > $(top_srcdir)/include/odp/api/spec/debug.h \ > + $(top_srcdir)/include/odp/api/spec/deprecated.h \ > $(top_srcdir)/include/odp/api/spec/errno.h \ > $(top_srcdir)/include/odp/api/spec/event.h \ > $(top_srcdir)/include/odp/api/spec/hash.h \ > diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am > index deab284..242c54a 100644 > --- a/platform/linux-generic/Makefile.am > +++ b/platform/linux-generic/Makefile.am > @@ -34,6 +34,7 @@ odpapiinclude_HEADERS = \ > $(srcdir)/include/odp/api/cpumask.h \ > $(srcdir)/include/odp/api/crypto.h \ > $(srcdir)/include/odp/api/debug.h \ > + $(srcdir)/include/odp/api/deprecated.h \ > $(srcdir)/include/odp/api/errno.h \ > $(srcdir)/include/odp/api/event.h \ > $(srcdir)/include/odp/api/hash.h \ > diff --git a/platform/linux-generic/include/odp/api/deprecated.h b/platform/linux-generic/include/odp/api/deprecated.h > new file mode 100644 > index 0000000..82797eb > --- /dev/null > +++ b/platform/linux-generic/include/odp/api/deprecated.h > @@ -0,0 +1,26 @@ > +/* Copyright (c) 2017, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +/** > + * @file > + * > + * Control deprecated API definitions > + */ > + > +#ifndef ODP_PLAT_DEPRECATED_H_ > +#define ODP_PLAT_DEPRECATED_H_ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#include <odp/api/spec/deprecated.h> > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif > -- > 2.8.1 >
I have to agree with Brian here. Cluttering up the API spec files with #ifdefs seems ugly and unnecessary, and tying this into autotools at a spec level also seems awkward. The idea was to mark certain fields as deprecated in one release and then remove them in a follow-on release. We can make the actual removal subject to SC vote if the concern is that things might get removed too soon, as there's no harm in carrying them with the deprecated markings until such time as there is agreement they really can be removed. Allowing individual developers to "reach back" to resurrect them just means that they have to be kept around forever as they're never really removed. On Thu, Feb 23, 2017 at 11:13 AM, Brian Brooks <brian.brooks@linaro.org> wrote: > On Thu, Feb 23, 2017 at 8:14 AM, Petri Savolainen > <petri.savolainen@linaro.org> wrote: > > Added ODP_DEPRECATED macro which is visible through API and > > is used to control if deprecated API definitions are enabled > > or disabled. > > > > Deprecated definitions are first moved behind this macro and > > then removed completely in a later API version. Deprecated APIs > > are disabled by default. > > > > Added configuration option --enable-deprecated to control the > > macro value. > > > > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > > --- > > configure.ac | 19 +++++++++-- > > include/odp/api/spec/.gitignore | 1 + > > include/odp/api/spec/deprecated.h.in | 38 > ++++++++++++++++++++++ > > include/odp_api.h | 1 + > > platform/Makefile.inc | 1 + > > platform/linux-generic/Makefile.am | 1 + > > .../linux-generic/include/odp/api/deprecated.h | 26 +++++++++++++++ > > 7 files changed, 85 insertions(+), 2 deletions(-) > > create mode 100644 include/odp/api/spec/deprecated.h.in > > create mode 100644 platform/linux-generic/include/odp/api/deprecated.h > > > > diff --git a/configure.ac b/configure.ac > > index e4bd3a7..9927916 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -16,7 +16,8 @@ ODP_VERSION_API_MAJOR=odpapi_major_version > > AC_SUBST(ODP_VERSION_API_MAJOR) > > ODP_VERSION_API_MINOR=odpapi_minor_version > > AC_SUBST(ODP_VERSION_API_MINOR) > > -AC_CONFIG_FILES([include/odp/api/spec/version.h]) > > +AC_CONFIG_FILES([include/odp/api/spec/version.h > > + include/odp/api/spec/deprecated.h]) > > > > AM_INIT_AUTOMAKE([1.9 tar-pax subdir-objects]) > > AC_CONFIG_SRCDIR([helper/config.h.in]) > > @@ -284,7 +285,7 @@ ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG" > > ODP_ABI_COMPAT=1 > > abi_compat=yes > > AC_ARG_ENABLE([abi-compat], > > - [ --disable-abi-compat disables ABI compatible mode, enables > inline code in header files], > > + [ --disable-abi-compat disables ABI compatible mode, enables > inline code in header files], > > [if test "x$enableval" = "xno"; then > > ODP_ABI_COMPAT=0 > > abi_compat=no > > @@ -294,6 +295,19 @@ AC_ARG_ENABLE([abi-compat], > > AC_SUBST(ODP_ABI_COMPAT) > > > > ############################################################ > ############## > > +# Enable/disable deprecated API definitions > > +########################################################### > ############### > > +ODP_DEPRECATED=0 > > +deprecated=no > > +AC_ARG_ENABLE([deprecated], > > + [ --enable-deprecated enable deprecated API definitions], > > + [if test "x$enableval" = "xyes"; then > > + ODP_DEPRECATED=1 > > + deprecated=yes > > + fi]) > > +AC_SUBST(ODP_DEPRECATED) > > + > > +########################################################### > ############### > > # Default warning setup > > ############################################################ > ############## > > ODP_CFLAGS="$ODP_CFLAGS -W -Wall -Werror -Wstrict-prototypes > -Wmissing-prototypes" > > @@ -379,6 +393,7 @@ AC_MSG_RESULT([ > > static libraries: ${enable_static} > > shared libraries: ${enable_shared} > > ABI compatible: ${abi_compat} > > + Deprecated APIs: ${deprecated} > > cunit: ${cunit_support} > > test_vald: ${test_vald} > > test_perf: ${test_perf} > > diff --git a/include/odp/api/spec/.gitignore b/include/odp/api/spec/. > gitignore > > index 6702033..df9c87d 100644 > > --- a/include/odp/api/spec/.gitignore > > +++ b/include/odp/api/spec/.gitignore > > @@ -1 +1,2 @@ > > +deprecated.h > > version.h > > diff --git a/include/odp/api/spec/deprecated.h.in > b/include/odp/api/spec/deprecated.h.in > > new file mode 100644 > > index 0000000..a319064 > > --- /dev/null > > +++ b/include/odp/api/spec/deprecated.h.in > > @@ -0,0 +1,38 @@ > > +/* Copyright (c) 2017, Linaro Limited > > + * All rights reserved. > > + * > > + * SPDX-License-Identifier: BSD-3-Clause > > + */ > > + > > +/** > > + * @file > > + * > > + * Macro for deprecated API definitions > > + */ > > + > > +#ifndef ODP_API_DEPRECATED_H_ > > +#define ODP_API_DEPRECATED_H_ > > +#include <odp/visibility_begin.h> > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +/** > > + * Deprecated API definitions > > + * > > + * Some API definitions may be deprecated by this or a previous API > version. > > + * This macro controls if those are enabled (and visible to the > application) > > + * or disabled. > > + * > > + * * 0: Deprecated API definitions are disabled > > + * * 1: Deprecated API definitions are enabled > > + */ > > +#define ODP_DEPRECATED @ODP_DEPRECATED@ > > I am confused here because this turns a compiler dependency into a > build system + compiler dependency. > > Lots of libraries choose to use Autotools as a build system, but it's > unnatural > to require API header files like odp.h to also use Autotools. Anyone can > grab > these API header files and: include them in their application to use ODP, > to > create stubs for an ODP implementation, and most importantly they should > be usable with any build system (Makefile, Autotools, CMake, ninja...). > > The original problem stated here is that the current code for the > deprecated > macro requires the use of GCC. The fix, or the way to address > portability, can > simply be to conditionally compile different code for different compilers: > > #ifdef __GCC__ > #define DEPRECATED __attribute__((deprecated)) > #elif MSCV > #define DEPRECATED __declspec(deprecated) > #else > // define it to nothing... a nop... > #define DEPRECATED > // or... if an implementation must be required for all compilers: > #error Please add support for your compiler in compiler.h > #endif > > It is succinct an easy to maintain compared to embedding these portability > concerns deeper into areas such as the build system and API header files. > > The same approach to portability can be used for architecturally > specific C code as well as any other system specific macros provided by > the compiler. > > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#include <odp/visibility_end.h> > > +#endif > > diff --git a/include/odp_api.h b/include/odp_api.h > > index 73e5309..962415f 100644 > > --- a/include/odp_api.h > > +++ b/include/odp_api.h > > @@ -18,6 +18,7 @@ > > extern "C" { > > #endif > > > > +#include <odp/api/deprecated.h> > > #include <odp/api/version.h> > > #include <odp/api/std_types.h> > > #include <odp/api/compiler.h> > > diff --git a/platform/Makefile.inc b/platform/Makefile.inc > > index 874cf88..f282770 100644 > > --- a/platform/Makefile.inc > > +++ b/platform/Makefile.inc > > @@ -29,6 +29,7 @@ odpapispecinclude_HEADERS = \ > > $(top_srcdir)/include/odp/api/spec/cpumask.h \ > > $(top_srcdir)/include/odp/api/spec/crypto.h \ > > $(top_srcdir)/include/odp/api/spec/debug.h \ > > + $(top_srcdir)/include/odp/api/spec/deprecated.h \ > > $(top_srcdir)/include/odp/api/spec/errno.h \ > > $(top_srcdir)/include/odp/api/spec/event.h \ > > $(top_srcdir)/include/odp/api/spec/hash.h \ > > diff --git a/platform/linux-generic/Makefile.am > b/platform/linux-generic/Makefile.am > > index deab284..242c54a 100644 > > --- a/platform/linux-generic/Makefile.am > > +++ b/platform/linux-generic/Makefile.am > > @@ -34,6 +34,7 @@ odpapiinclude_HEADERS = \ > > $(srcdir)/include/odp/api/cpumask.h \ > > $(srcdir)/include/odp/api/crypto.h \ > > $(srcdir)/include/odp/api/debug.h \ > > + $(srcdir)/include/odp/api/deprecated.h \ > > $(srcdir)/include/odp/api/errno.h \ > > $(srcdir)/include/odp/api/event.h \ > > $(srcdir)/include/odp/api/hash.h \ > > diff --git a/platform/linux-generic/include/odp/api/deprecated.h > b/platform/linux-generic/include/odp/api/deprecated.h > > new file mode 100644 > > index 0000000..82797eb > > --- /dev/null > > +++ b/platform/linux-generic/include/odp/api/deprecated.h > > @@ -0,0 +1,26 @@ > > +/* Copyright (c) 2017, Linaro Limited > > + * All rights reserved. > > + * > > + * SPDX-License-Identifier: BSD-3-Clause > > + */ > > + > > +/** > > + * @file > > + * > > + * Control deprecated API definitions > > + */ > > + > > +#ifndef ODP_PLAT_DEPRECATED_H_ > > +#define ODP_PLAT_DEPRECATED_H_ > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +#include <odp/api/spec/deprecated.h> > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif > > -- > > 2.8.1 > > >
From: Bill Fischofer [mailto:bill.fischofer@linaro.org]
Sent: Thursday, February 23, 2017 7:28 PM
To: Brian Brooks <brian.brooks@linaro.org>
Cc: Petri Savolainen <petri.savolainen@linaro.org>; LNG ODP Mailman List <lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] [API-NEXT PATCH 2/4] api: deprecated: add ODP_DEPRECATED configure option
I have to agree with Brian here. Cluttering up the API spec files with #ifdefs seems ugly and unnecessary, and tying this into autotools at a spec level also seems awkward.
The idea was to mark certain fields as deprecated in one release and then remove them in a follow-on release. We can make the actual removal subject to SC vote if the concern is that things might get removed too soon, as there's no harm in carrying them with the deprecated markings until such time as there is agreement they really can be removed. Allowing individual developers to "reach back" to resurrect them just means that they have to
be kept around forever as they're never really removed.
NO HTML mails please
--------------------
First, this design pattern is copied from our version.h.in API file:
/**
* ODP API generation version
*/
#define ODP_VERSION_API_GENERATION @ODP_VERSION_API_GENERATION@
xxx.h.in files are not part API, but the generated xxx.h files are. After autotools have done it's work, there's include/odp/api/spec/deprecated.h file which has this content:
/**
* Deprecated API definitions
*
* Some API definitions may be deprecated by this or a previous API version.
* This macro controls if those are enabled (and visible to the application)
* or disabled.
*
* * 0: Deprecated API definitions are disabled
* * 1: Deprecated API definitions are enabled
*/
#define ODP_DEPRECATED 0
If an implementation does not use autotools, it may generate the same deprecated.h file with the same content anyway it likes (or hard code the file into the repo).
Second, this patch does not take any position how long a deprecated API remains there. The patch enables application build *with* or *without* deprecated APIs, so that it's easy to verify if deprecated APIs are (still) used in application or not. For example, I missed couple of those in IPSEC example app porting previously, but now with this option those were caught. Also implementation may be more efficient when deprecated code is removed.
-Petri
> > +/** > > + * Deprecated API definitions > > + * > > + * Some API definitions may be deprecated by this or a previous API > version. > > + * This macro controls if those are enabled (and visible to the > application) > > + * or disabled. > > + * > > + * * 0: Deprecated API definitions are disabled > > + * * 1: Deprecated API definitions are enabled > > + */ > > +#define ODP_DEPRECATED @ODP_DEPRECATED@ > > I am confused here because this turns a compiler dependency into a > build system + compiler dependency. > > Lots of libraries choose to use Autotools as a build system, but it's > unnatural > to require API header files like odp.h to also use Autotools. Anyone can > grab > these API header files and: include them in their application to use ODP, > to > create stubs for an ODP implementation, and most importantly they should > be usable with any build system (Makefile, Autotools, CMake, ninja...). See version.h.in, it has the same design. Implementations can generate version.h/deprecated.h in various ways. > > The original problem stated here is that the current code for the > deprecated > macro requires the use of GCC. The fix, or the way to address > portability, can > simply be to conditionally compile different code for different compilers: The original problem is to reliably remove deprecated API defienes/types/struct fields/function calls/etc from application, and all unnecessary code from an implementation. GCC __attribute__ is not suitable for all that. -Petri
On Fri, Feb 24, 2017 at 2:30 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > From: Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Thursday, February 23, 2017 7:28 PM > To: Brian Brooks <brian.brooks@linaro.org> > Cc: Petri Savolainen <petri.savolainen@linaro.org>; LNG ODP Mailman List <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCH 2/4] api: deprecated: add ODP_DEPRECATED configure option > > I have to agree with Brian here. Cluttering up the API spec files with #ifdefs seems ugly and unnecessary, and tying this into autotools at a spec level also seems awkward. > > The idea was to mark certain fields as deprecated in one release and then remove them in a follow-on release. We can make the actual removal subject to SC vote if the concern is that things might get removed too soon, as there's no harm in carrying them with the deprecated markings until such time as there is agreement they really can be removed. Allowing individual developers to "reach back" to resurrect them just means that they have to > be kept around forever as they're never really removed. > > > > NO HTML mails please > -------------------- > > First, this design pattern is copied from our version.h.in API file: OK > /** > * ODP API generation version > */ > #define ODP_VERSION_API_GENERATION @ODP_VERSION_API_GENERATION@ > > > xxx.h.in files are not part API, but the generated xxx.h files are. After autotools have done it's work, there's include/odp/api/spec/deprecated.h file which has this content: > > /** > * Deprecated API definitions > * > * Some API definitions may be deprecated by this or a previous API version. > * This macro controls if those are enabled (and visible to the application) > * or disabled. > * > * * 0: Deprecated API definitions are disabled > * * 1: Deprecated API definitions are enabled > */ > #define ODP_DEPRECATED 0 > > If an implementation does not use autotools, it may generate the same deprecated.h file with the same content anyway it likes (or hard code the file into the repo). I disagree strongly. Such API header files should contain pure C code, and not anything that requires the file to be run through autoconf or for people to maintain their own set of API header files. Things like @ODP_VERSION_API_GENERATION@ can be defined to a real concrete value. Things like ODP_DEPRECATED can also be defined to a real value .. conditional on the compiler identification macro. > > Second, this patch does not take any position how long a deprecated API remains there. The patch enables application build *with* or *without* deprecated APIs, so that it's easy to verify if deprecated APIs are (still) used in application or not. For example, I missed couple of those in IPSEC example app porting previously, but now with this option those were caught. I am confused again. Use of the compiler's support for "deprecated" will cause a warning to be emitted if application code is still written to use said deprecated APIs. Are you saying this is not good enough? Or, are you trying to create a way to still support some idea of "deprecated" in the case that there is no support from the compiler? > Also implementation may be more efficient when deprecated code is removed. > > -Petri >
> > /** > > * Deprecated API definitions > > * > > * Some API definitions may be deprecated by this or a previous API > version. > > * This macro controls if those are enabled (and visible to the > application) > > * or disabled. > > * > > * * 0: Deprecated API definitions are disabled > > * * 1: Deprecated API definitions are enabled > > */ > > #define ODP_DEPRECATED 0 > > > > If an implementation does not use autotools, it may generate the same > deprecated.h file with the same content anyway it likes (or hard code the > file into the repo). > > I disagree strongly. Such API header files should contain pure C > code, and not anything that requires the file to be run through > autoconf or for people to maintain their own set of API header files. > Things like @ODP_VERSION_API_GENERATION@ can be defined to a real > concrete value. Things like ODP_DEPRECATED can also be defined to a > real value .. conditional on the compiler identification macro. This is not far from other values being implementation specific. For example, the API spec defines that there is ODP_QUEUE_INVALID defined, but does not dictate the value (an ABI spec does when building for ABI compat). An implementation needs to make sure that application sees the macro through odp_api.h. So, it needs to have a (build) mechanism for that. Similarly, API spec defines that ODP_DEPRECATED has value of 1, when deprecated APIs are present. Implementation needs to make that definition visible (somehow) through odp_api.h, when those APIs are enabled. Currently, in odp-linux, we use automake for that. > > > > > Second, this patch does not take any position how long a deprecated API > remains there. The patch enables application build *with* or *without* > deprecated APIs, so that it's easy to verify if deprecated APIs are > (still) used in application or not. For example, I missed couple of those > in IPSEC example app porting previously, but now with this option those > were caught. > > I am confused again. Use of the compiler's support for "deprecated" > will cause a warning to be emitted if application code is still > written to use said deprecated APIs. Are you saying this is not good > enough? Or, are you trying to create a way to still support some idea > of "deprecated" in the case that there is no support from the > compiler? It's not good enough, and it depends on an non-standard compiler feature when that dependency is not absolutely mandatory for what we are trying to achieve. 1) New API version. Some definition is now deprecated. 2) By default, --enable-deprecated=no. Old definitions are #ifdef'ed away. Application build fails if it still uses a deprecated definition. 3.1) Update the application to use new API definitions. This gives best performance, OR ... 3.2) Don't change application this time, but build it with --enable-deprecated=yes. Application builds, but may suffer from lower performance as implementation needs to include both code path the old and the new. The "new API" is now the default code path. The "old API" is likely to be less used/tested/supported - it will be removed soon anyway. 4) New API version. The deprecated definition (deprecated in 1) above) may be now removed for good. All application need to upgrade. Implementation may delete the #ifdef'ed old code path. I think this is #if ODP_DEPRECATED is simple and strong way to enforce deprecation in API ... +#if ODP_DEPRECATED /** @deprecated Use aes_cbc instead */ uint32_t aes128_cbc : 1; /** @deprecated Use aes_gcm instead */ uint32_t aes128_gcm : 1; +#endif ... and gives implementation chance to optimize for the new API (the new default). +#if ODP_DEPRECATED case ODP_AUTH_ALG_AES128_GCM: + if (param->cipher_alg == ODP_CIPHER_ALG_AES128_GCM) + aes_gcm = 1; + /* Fallthrough */ +#endif + case ODP_AUTH_ALG_AES_GCM: /* AES-GCM requires to do both auth and * cipher at the same time */ - if (param->cipher_alg == ODP_CIPHER_ALG_AES_GCM || - param->cipher_alg == ODP_CIPHER_ALG_AES128_GCM) { + if (param->cipher_alg == ODP_CIPHER_ALG_AES_GCM || aes_gcm) { session->auth.func = null_crypto_routine; In this example above, #if ODP_DEPRECATED allows implementation to do if(x == y || 0), instead of if(x == y || x == z). Effectively, one compare instead of two. I think it's better to clearly select: either deprecated APIs are enabled or disabled. So, that both application and implementation know which spec is used (the old or the new). -Petri
diff --git a/configure.ac b/configure.ac index e4bd3a7..9927916 100644 --- a/configure.ac +++ b/configure.ac @@ -16,7 +16,8 @@ ODP_VERSION_API_MAJOR=odpapi_major_version AC_SUBST(ODP_VERSION_API_MAJOR) ODP_VERSION_API_MINOR=odpapi_minor_version AC_SUBST(ODP_VERSION_API_MINOR) -AC_CONFIG_FILES([include/odp/api/spec/version.h]) +AC_CONFIG_FILES([include/odp/api/spec/version.h + include/odp/api/spec/deprecated.h]) AM_INIT_AUTOMAKE([1.9 tar-pax subdir-objects]) AC_CONFIG_SRCDIR([helper/config.h.in]) @@ -284,7 +285,7 @@ ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG" ODP_ABI_COMPAT=1 abi_compat=yes AC_ARG_ENABLE([abi-compat], - [ --disable-abi-compat disables ABI compatible mode, enables inline code in header files], + [ --disable-abi-compat disables ABI compatible mode, enables inline code in header files], [if test "x$enableval" = "xno"; then ODP_ABI_COMPAT=0 abi_compat=no @@ -294,6 +295,19 @@ AC_ARG_ENABLE([abi-compat], AC_SUBST(ODP_ABI_COMPAT) ########################################################################## +# Enable/disable deprecated API definitions +########################################################################## +ODP_DEPRECATED=0 +deprecated=no +AC_ARG_ENABLE([deprecated], + [ --enable-deprecated enable deprecated API definitions], + [if test "x$enableval" = "xyes"; then + ODP_DEPRECATED=1 + deprecated=yes + fi]) +AC_SUBST(ODP_DEPRECATED) + +########################################################################## # Default warning setup ########################################################################## ODP_CFLAGS="$ODP_CFLAGS -W -Wall -Werror -Wstrict-prototypes -Wmissing-prototypes" @@ -379,6 +393,7 @@ AC_MSG_RESULT([ static libraries: ${enable_static} shared libraries: ${enable_shared} ABI compatible: ${abi_compat} + Deprecated APIs: ${deprecated} cunit: ${cunit_support} test_vald: ${test_vald} test_perf: ${test_perf} diff --git a/include/odp/api/spec/.gitignore b/include/odp/api/spec/.gitignore index 6702033..df9c87d 100644 --- a/include/odp/api/spec/.gitignore +++ b/include/odp/api/spec/.gitignore @@ -1 +1,2 @@ +deprecated.h version.h diff --git a/include/odp/api/spec/deprecated.h.in b/include/odp/api/spec/deprecated.h.in new file mode 100644 index 0000000..a319064 --- /dev/null +++ b/include/odp/api/spec/deprecated.h.in @@ -0,0 +1,38 @@ +/* Copyright (c) 2017, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * Macro for deprecated API definitions + */ + +#ifndef ODP_API_DEPRECATED_H_ +#define ODP_API_DEPRECATED_H_ +#include <odp/visibility_begin.h> + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * Deprecated API definitions + * + * Some API definitions may be deprecated by this or a previous API version. + * This macro controls if those are enabled (and visible to the application) + * or disabled. + * + * * 0: Deprecated API definitions are disabled + * * 1: Deprecated API definitions are enabled + */ +#define ODP_DEPRECATED @ODP_DEPRECATED@ + +#ifdef __cplusplus +} +#endif + +#include <odp/visibility_end.h> +#endif diff --git a/include/odp_api.h b/include/odp_api.h index 73e5309..962415f 100644 --- a/include/odp_api.h +++ b/include/odp_api.h @@ -18,6 +18,7 @@ extern "C" { #endif +#include <odp/api/deprecated.h> #include <odp/api/version.h> #include <odp/api/std_types.h> #include <odp/api/compiler.h> diff --git a/platform/Makefile.inc b/platform/Makefile.inc index 874cf88..f282770 100644 --- a/platform/Makefile.inc +++ b/platform/Makefile.inc @@ -29,6 +29,7 @@ odpapispecinclude_HEADERS = \ $(top_srcdir)/include/odp/api/spec/cpumask.h \ $(top_srcdir)/include/odp/api/spec/crypto.h \ $(top_srcdir)/include/odp/api/spec/debug.h \ + $(top_srcdir)/include/odp/api/spec/deprecated.h \ $(top_srcdir)/include/odp/api/spec/errno.h \ $(top_srcdir)/include/odp/api/spec/event.h \ $(top_srcdir)/include/odp/api/spec/hash.h \ diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am index deab284..242c54a 100644 --- a/platform/linux-generic/Makefile.am +++ b/platform/linux-generic/Makefile.am @@ -34,6 +34,7 @@ odpapiinclude_HEADERS = \ $(srcdir)/include/odp/api/cpumask.h \ $(srcdir)/include/odp/api/crypto.h \ $(srcdir)/include/odp/api/debug.h \ + $(srcdir)/include/odp/api/deprecated.h \ $(srcdir)/include/odp/api/errno.h \ $(srcdir)/include/odp/api/event.h \ $(srcdir)/include/odp/api/hash.h \ diff --git a/platform/linux-generic/include/odp/api/deprecated.h b/platform/linux-generic/include/odp/api/deprecated.h new file mode 100644 index 0000000..82797eb --- /dev/null +++ b/platform/linux-generic/include/odp/api/deprecated.h @@ -0,0 +1,26 @@ +/* Copyright (c) 2017, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * Control deprecated API definitions + */ + +#ifndef ODP_PLAT_DEPRECATED_H_ +#define ODP_PLAT_DEPRECATED_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +#include <odp/api/spec/deprecated.h> + +#ifdef __cplusplus +} +#endif + +#endif
Added ODP_DEPRECATED macro which is visible through API and is used to control if deprecated API definitions are enabled or disabled. Deprecated definitions are first moved behind this macro and then removed completely in a later API version. Deprecated APIs are disabled by default. Added configuration option --enable-deprecated to control the macro value. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- configure.ac | 19 +++++++++-- include/odp/api/spec/.gitignore | 1 + include/odp/api/spec/deprecated.h.in | 38 ++++++++++++++++++++++ include/odp_api.h | 1 + platform/Makefile.inc | 1 + platform/linux-generic/Makefile.am | 1 + .../linux-generic/include/odp/api/deprecated.h | 26 +++++++++++++++ 7 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 include/odp/api/spec/deprecated.h.in create mode 100644 platform/linux-generic/include/odp/api/deprecated.h -- 2.8.1