| Message ID | 1449677301-3254-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
This is proposing an API change. It's OK to change how odp_time_null() is implemented in linux-generic, but we can't arbitrarily remove this public API from the linux-generic implementation. On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > No need to define odp_time_null() as it only initialize to 0 > time. Remove it completely with needed to add doxygen comment > for it. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/odp/plat/time_types.h | 4 +--- > platform/linux-generic/odp_time.c | 7 +------ > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/platform/linux-generic/include/odp/plat/time_types.h > b/platform/linux-generic/include/odp/plat/time_types.h > index e5765ec..10751f5 100644 > --- a/platform/linux-generic/include/odp/plat/time_types.h > +++ b/platform/linux-generic/include/odp/plat/time_types.h > @@ -23,9 +23,7 @@ extern "C" { > > typedef struct timespec odp_time_t; > > -odp_time_t odp_time_null(void); > - > -#define ODP_TIME_NULL odp_time_null() > +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) > > /** > * @} > diff --git a/platform/linux-generic/odp_time.c > b/platform/linux-generic/odp_time.c > index 1c7c214..1d374ca 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t time) > return time_to_ns(time) / resolution; > } > > -odp_time_t odp_time_null(void) > -{ > - return (struct timespec) {0, 0}; > -} > - > int odp_time_global_init(void) > { > int ret; > struct timespec time; > > ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); > - start_time = ret ? odp_time_null() : time; > + start_time = ret ? ODP_TIME_NULL : time; > > return ret; > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 12/09/2015 19:26, Bill Fischofer wrote: > This is proposing an API change. It's OK to change how > odp_time_null() is implemented in linux-generic, but we can't > arbitrarily remove this public API from the linux-generic implementation. But it's not defined in API headers: fgrep -r odp_time_null ./include/ <nothing> Also all validation tests use ODP_TIME_NULL instead of odp_time_null(). There is no need to duplicated things. Maxim. > > On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > No need to define odp_time_null() as it only initialize to 0 > time. Remove it completely with needed to add doxygen comment > for it. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/include/odp/plat/time_types.h | 4 +--- > platform/linux-generic/odp_time.c | 7 +------ > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/platform/linux-generic/include/odp/plat/time_types.h > b/platform/linux-generic/include/odp/plat/time_types.h > index e5765ec..10751f5 100644 > --- a/platform/linux-generic/include/odp/plat/time_types.h > +++ b/platform/linux-generic/include/odp/plat/time_types.h > @@ -23,9 +23,7 @@ extern "C" { > > typedef struct timespec odp_time_t; > > -odp_time_t odp_time_null(void); > - > -#define ODP_TIME_NULL odp_time_null() > +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) > > /** > * @} > diff --git a/platform/linux-generic/odp_time.c > b/platform/linux-generic/odp_time.c > index 1c7c214..1d374ca 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t time) > return time_to_ns(time) / resolution; > } > > -odp_time_t odp_time_null(void) > -{ > - return (struct timespec) {0, 0}; > -} > - > int odp_time_global_init(void) > { > int ret; > struct timespec time; > > ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); > - start_time = ret ? odp_time_null() : time; > + start_time = ret ? ODP_TIME_NULL : time; > > return ret; > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
Sorry, my mistake. I had assumed that using odp_time_null() rather than _odp_time_null() was because it was implementing an external API. However that does raise the question of whether odp_time_null() or ODP_TIME_NULL should be part of the external API. On Wed, Dec 9, 2015 at 10:29 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/09/2015 19:26, Bill Fischofer wrote: > >> This is proposing an API change. It's OK to change how odp_time_null() >> is implemented in linux-generic, but we can't arbitrarily remove this >> public API from the linux-generic implementation. >> > > But it's not defined in API headers: > fgrep -r odp_time_null ./include/ > <nothing> > > Also all validation tests use ODP_TIME_NULL instead of odp_time_null(). > There is no need to duplicated things. > > Maxim. > > >> On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> No need to define odp_time_null() as it only initialize to 0 >> time. Remove it completely with needed to add doxygen comment >> for it. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> >> --- >> platform/linux-generic/include/odp/plat/time_types.h | 4 +--- >> platform/linux-generic/odp_time.c | 7 +------ >> 2 files changed, 2 insertions(+), 9 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp/plat/time_types.h >> b/platform/linux-generic/include/odp/plat/time_types.h >> index e5765ec..10751f5 100644 >> --- a/platform/linux-generic/include/odp/plat/time_types.h >> +++ b/platform/linux-generic/include/odp/plat/time_types.h >> @@ -23,9 +23,7 @@ extern "C" { >> >> typedef struct timespec odp_time_t; >> >> -odp_time_t odp_time_null(void); >> - >> -#define ODP_TIME_NULL odp_time_null() >> +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) >> >> /** >> * @} >> diff --git a/platform/linux-generic/odp_time.c >> b/platform/linux-generic/odp_time.c >> index 1c7c214..1d374ca 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t time) >> return time_to_ns(time) / resolution; >> } >> >> -odp_time_t odp_time_null(void) >> -{ >> - return (struct timespec) {0, 0}; >> -} >> - >> int odp_time_global_init(void) >> { >> int ret; >> struct timespec time; >> >> ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> - start_time = ret ? odp_time_null() : time; >> + start_time = ret ? ODP_TIME_NULL : time; >> >> return ret; >> } >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
On 12/09/2015 19:33, Bill Fischofer wrote: > Sorry, my mistake. I had assumed that using odp_time_null() rather > than _odp_time_null() was because it was implementing an external > API. However that does raise the question of whether odp_time_null() > or ODP_TIME_NULL should be part of the external API. I don't understand use case for that. Usually current time make sense. Or some time in past or future. But zero time looks like useless. Maxim. > > On Wed, Dec 9, 2015 at 10:29 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 12/09/2015 19:26, Bill Fischofer wrote: > > This is proposing an API change. It's OK to change how > odp_time_null() is implemented in linux-generic, but we can't > arbitrarily remove this public API from the linux-generic > implementation. > > > But it's not defined in API headers: > fgrep -r odp_time_null ./include/ > <nothing> > > Also all validation tests use ODP_TIME_NULL instead of > odp_time_null(). There is no need to duplicated things. > > Maxim. > > > On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > No need to define odp_time_null() as it only initialize to 0 > time. Remove it completely with needed to add doxygen comment > for it. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > > --- > platform/linux-generic/include/odp/plat/time_types.h | 4 +--- > platform/linux-generic/odp_time.c | 7 +------ > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git > a/platform/linux-generic/include/odp/plat/time_types.h > b/platform/linux-generic/include/odp/plat/time_types.h > index e5765ec..10751f5 100644 > --- a/platform/linux-generic/include/odp/plat/time_types.h > +++ b/platform/linux-generic/include/odp/plat/time_types.h > @@ -23,9 +23,7 @@ extern "C" { > > typedef struct timespec odp_time_t; > > -odp_time_t odp_time_null(void); > - > -#define ODP_TIME_NULL odp_time_null() > +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) > > /** > * @} > diff --git a/platform/linux-generic/odp_time.c > b/platform/linux-generic/odp_time.c > index 1c7c214..1d374ca 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t > time) > return time_to_ns(time) / resolution; > } > > -odp_time_t odp_time_null(void) > -{ > - return (struct timespec) {0, 0}; > -} > - > int odp_time_global_init(void) > { > int ret; > struct timespec time; > > ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); > - start_time = ret ? odp_time_null() : time; > + start_time = ret ? ODP_TIME_NULL : time; > > return ret; > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > >
On 09.12.15 20:24, Maxim Uvarov wrote: > On 12/09/2015 19:33, Bill Fischofer wrote: >> Sorry, my mistake. I had assumed that using odp_time_null() rather than _odp_time_null() was because it was implementing an external API. However that does raise the question of whether odp_time_null() or ODP_TIME_NULL should be part of the external API. > > I don't understand use case for that. Usually current time make sense. Or some time in past or future. But zero time looks like useless. > > Maxim. Initialization, comparison and validation. > >> >> On Wed, Dec 9, 2015 at 10:29 AM, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 12/09/2015 19:26, Bill Fischofer wrote: >> >> This is proposing an API change. It's OK to change how >> odp_time_null() is implemented in linux-generic, but we can't >> arbitrarily remove this public API from the linux-generic >> implementation. >> >> >> But it's not defined in API headers: >> fgrep -r odp_time_null ./include/ >> <nothing> >> >> Also all validation tests use ODP_TIME_NULL instead of >> odp_time_null(). There is no need to duplicated things. >> >> Maxim. >> >> >> On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> No need to define odp_time_null() as it only initialize to 0 >> time. Remove it completely with needed to add doxygen comment >> for it. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> >> >> --- >> platform/linux-generic/include/odp/plat/time_types.h | 4 +--- >> platform/linux-generic/odp_time.c | 7 +------ >> 2 files changed, 2 insertions(+), 9 deletions(-) >> >> diff --git >> a/platform/linux-generic/include/odp/plat/time_types.h >> b/platform/linux-generic/include/odp/plat/time_types.h >> index e5765ec..10751f5 100644 >> --- a/platform/linux-generic/include/odp/plat/time_types.h >> +++ b/platform/linux-generic/include/odp/plat/time_types.h >> @@ -23,9 +23,7 @@ extern "C" { >> >> typedef struct timespec odp_time_t; >> >> -odp_time_t odp_time_null(void); >> - >> -#define ODP_TIME_NULL odp_time_null() >> +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) >> >> /** >> * @} >> diff --git a/platform/linux-generic/odp_time.c >> b/platform/linux-generic/odp_time.c >> index 1c7c214..1d374ca 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t >> time) >> return time_to_ns(time) / resolution; >> } >> >> -odp_time_t odp_time_null(void) >> -{ >> - return (struct timespec) {0, 0}; >> -} >> - >> int odp_time_global_init(void) >> { >> int ret; >> struct timespec time; >> >> ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> - start_time = ret ? odp_time_null() : time; >> + start_time = ret ? ODP_TIME_NULL : time; >> >> return ret; >> } >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
On 09.12.15 18:33, Bill Fischofer wrote: > Sorry, my mistake. I had assumed that using odp_time_null() rather than _odp_time_null() was because it was implementing an external API > However that does raise the question of whether odp_time_null() or ODP_TIME_NULL should be part of the external API. Yep. In most cases it's simple var 0,0 and in this case usage of function seems a redundancy. > > On Wed, Dec 9, 2015 at 10:29 AM, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: > > On 12/09/2015 19:26, Bill Fischofer wrote: > > This is proposing an API change. It's OK to change how odp_time_null() is implemented in linux-generic, but we can't arbitrarily remove this public API from the linux-generic implementation. > > > But it's not defined in API headers: > fgrep -r odp_time_null ./include/ > <nothing> > > Also all validation tests use ODP_TIME_NULL instead of odp_time_null(). There is no need to duplicated things. > > Maxim. > > > On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> <mailto:maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>>> wrote: > > No need to define odp_time_null() as it only initialize to 0 > time. Remove it completely with needed to add doxygen comment > for it. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>>> > > --- > platform/linux-generic/include/odp/plat/time_types.h | 4 +--- > platform/linux-generic/odp_time.c | 7 +------ > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/platform/linux-generic/include/odp/plat/time_types.h > b/platform/linux-generic/include/odp/plat/time_types.h > index e5765ec..10751f5 100644 > --- a/platform/linux-generic/include/odp/plat/time_types.h > +++ b/platform/linux-generic/include/odp/plat/time_types.h > @@ -23,9 +23,7 @@ extern "C" { > > typedef struct timespec odp_time_t; > > -odp_time_t odp_time_null(void); > - > -#define ODP_TIME_NULL odp_time_null() > +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) > > /** > * @} > diff --git a/platform/linux-generic/odp_time.c > b/platform/linux-generic/odp_time.c > index 1c7c214..1d374ca 100644 > --- a/platform/linux-generic/odp_time.c > +++ b/platform/linux-generic/odp_time.c > @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t time) > return time_to_ns(time) / resolution; > } > > -odp_time_t odp_time_null(void) > -{ > - return (struct timespec) {0, 0}; > -} > - > int odp_time_global_init(void) > { > int ret; > struct timespec time; > > ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); > - start_time = ret ? odp_time_null() : time; > + start_time = ret ? ODP_TIME_NULL : time; > > return ret; > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> <mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 09.12.15 18:29, Maxim Uvarov wrote: > On 12/09/2015 19:26, Bill Fischofer wrote: >> This is proposing an API change. It's OK to change how odp_time_null() is implemented in linux-generic, but we can't arbitrarily remove this public API from the linux-generic implementation. > > But it's not defined in API headers: > fgrep -r odp_time_null ./include/ > <nothing> > > Also all validation tests use ODP_TIME_NULL instead of odp_time_null(). There is no need to duplicated things. > > Maxim. odp_time_null() was supposed to be linux-generic internal function. Can be prefixed or deleted. > >> >> On Wed, Dec 9, 2015 at 10:08 AM, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >> >> No need to define odp_time_null() as it only initialize to 0 >> time. Remove it completely with needed to add doxygen comment >> for it. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> --- >> platform/linux-generic/include/odp/plat/time_types.h | 4 +--- >> platform/linux-generic/odp_time.c | 7 +------ >> 2 files changed, 2 insertions(+), 9 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp/plat/time_types.h >> b/platform/linux-generic/include/odp/plat/time_types.h >> index e5765ec..10751f5 100644 >> --- a/platform/linux-generic/include/odp/plat/time_types.h >> +++ b/platform/linux-generic/include/odp/plat/time_types.h >> @@ -23,9 +23,7 @@ extern "C" { >> >> typedef struct timespec odp_time_t; >> >> -odp_time_t odp_time_null(void); >> - >> -#define ODP_TIME_NULL odp_time_null() >> +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) >> >> /** >> * @} >> diff --git a/platform/linux-generic/odp_time.c >> b/platform/linux-generic/odp_time.c >> index 1c7c214..1d374ca 100644 >> --- a/platform/linux-generic/odp_time.c >> +++ b/platform/linux-generic/odp_time.c >> @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t time) >> return time_to_ns(time) / resolution; >> } >> >> -odp_time_t odp_time_null(void) >> -{ >> - return (struct timespec) {0, 0}; >> -} >> - >> int odp_time_global_init(void) >> { >> int ret; >> struct timespec time; >> >> ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); >> - start_time = ret ? odp_time_null() : time; >> + start_time = ret ? ODP_TIME_NULL : time; >> >> return ret; >> } >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/include/odp/plat/time_types.h b/platform/linux-generic/include/odp/plat/time_types.h index e5765ec..10751f5 100644 --- a/platform/linux-generic/include/odp/plat/time_types.h +++ b/platform/linux-generic/include/odp/plat/time_types.h @@ -23,9 +23,7 @@ extern "C" { typedef struct timespec odp_time_t; -odp_time_t odp_time_null(void); - -#define ODP_TIME_NULL odp_time_null() +#define ODP_TIME_NULL ((odp_time_t) {0, 0}) /** * @} diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c index 1c7c214..1d374ca 100644 --- a/platform/linux-generic/odp_time.c +++ b/platform/linux-generic/odp_time.c @@ -113,18 +113,13 @@ uint64_t odp_time_to_u64(odp_time_t time) return time_to_ns(time) / resolution; } -odp_time_t odp_time_null(void) -{ - return (struct timespec) {0, 0}; -} - int odp_time_global_init(void) { int ret; struct timespec time; ret = clock_gettime(CLOCK_MONOTONIC_RAW, &time); - start_time = ret ? odp_time_null() : time; + start_time = ret ? ODP_TIME_NULL : time; return ret; }
No need to define odp_time_null() as it only initialize to 0 time. Remove it completely with needed to add doxygen comment for it. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/include/odp/plat/time_types.h | 4 +--- platform/linux-generic/odp_time.c | 7 +------ 2 files changed, 2 insertions(+), 9 deletions(-)