| Message ID | 1465241288-5308-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
I disagree with this. The enum in this case is simply an alternative to using #defines. These values are in fact ints and ODP API standard practice is to return ints, not enums. I don't see any advantage to singling out these APIs for different treatment. One could argue that the "proper" thing to do here would be to return -1 and have the specific reason be returned via the odp_errno. But I don't think that change is needed either since we define <0 RCs to be error returns and its up to each API to decide if it wants to have multiple such error return values. On Mon, Jun 6, 2016 at 2:28 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > For some reason we defined function to set timer as int but have enum > in api. It looks more clear to bind return code to current api description. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > include/odp/api/spec/timer.h | 12 ++++++------ > platform/linux-generic/odp_timer.c | 12 ++++++------ > 2 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h > index 3f8fdd4..2c3bb8a 100644 > --- a/include/odp/api/spec/timer.h > +++ b/include/odp/api/spec/timer.h > @@ -243,9 +243,9 @@ odp_event_t odp_timer_free(odp_timer_t tim); > * @retval ODP_TIMER_NOEVENT Operation failed because timeout event not > * specified in odp_timer_set call and not present in timer > */ > -int odp_timer_set_abs(odp_timer_t tim, > - uint64_t abs_tck, > - odp_event_t *tmo_ev); > +odp_timer_set_t odp_timer_set_abs(odp_timer_t tim, > + uint64_t abs_tck, > + odp_event_t *tmo_ev); > > /** > * Set a timer with a relative expiration time and user-provided event. > @@ -268,9 +268,9 @@ int odp_timer_set_abs(odp_timer_t tim, > * @retval ODP_TIMER_NOEVENT Operation failed because timeout event not > * specified in call and not present in timer > */ > -int odp_timer_set_rel(odp_timer_t tim, > - uint64_t rel_tck, > - odp_event_t *tmo_ev); > +odp_timer_set_t odp_timer_set_rel(odp_timer_t tim, > + uint64_t rel_tck, > + odp_event_t *tmo_ev); > > /** > * Cancel a timer > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index 996edf0..595b6ab 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -865,9 +865,9 @@ odp_event_t odp_timer_free(odp_timer_t hdl) > return odp_buffer_to_event(old_buf); > } > > -int odp_timer_set_abs(odp_timer_t hdl, > - uint64_t abs_tck, > - odp_event_t *tmo_ev) > +odp_timer_set_t odp_timer_set_abs(odp_timer_t hdl, > + uint64_t abs_tck, > + odp_event_t *tmo_ev) > { > odp_timer_pool *tp = handle_to_tp(hdl); > uint32_t idx = handle_to_idx(hdl, tp); > @@ -882,9 +882,9 @@ int odp_timer_set_abs(odp_timer_t hdl, > return ODP_TIMER_NOEVENT; > } > > -int odp_timer_set_rel(odp_timer_t hdl, > - uint64_t rel_tck, > - odp_event_t *tmo_ev) > +odp_timer_set_t odp_timer_set_rel(odp_timer_t hdl, > + uint64_t rel_tck, > + odp_event_t *tmo_ev) > { > odp_timer_pool *tp = handle_to_tp(hdl); > uint32_t idx = handle_to_idx(hdl, tp); > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 06/06/16 23:16, Bill Fischofer wrote: > I disagree with this. The enum in this case is simply an alternative > to using #defines. These values are in fact ints and ODP API standard > practice is to return ints, not enums. I don't see any advantage to > singling out these APIs for different treatment. > > One could argue that the "proper" thing to do here would be to return > -1 and have the specific reason be returned via the odp_errno. But I > don't think that change is needed either since we define <0 RCs to be > error returns and its up to each API to decide if it wants to have > multiple such error return values. Bill, in doxygen description we did not define any <0 RCs, only that we have in enum. So if it's int probably we need to add: * @retval other != 0 return for other error */ /** * Set a timer (absolute time) with a user-provided timeout event * * Set (arm) the timer to expire at specific time. The timeout * event will be enqueued when the timer expires. * * @param tim Timer * @param abs_tck Expiration time in absolute timer ticks * @param[in,out] tmo_ev Reference to an event variable that points to * timeout event or NULL to reuse the existing timeout event. Any existing * timeout event that is replaced by a successful set operation will be * returned here. * * @retval ODP_TIMER_SUCCESS Operation succeeded * @retval ODP_TIMER_TOOEARLY Operation failed because expiration tick too * early * @retval ODP_TIMER_TOOLATE Operation failed because expiration tick too * late * @retval ODP_TIMER_NOEVENT Operation failed because timeout event not * specified in odp_timer_set call and not present in timer */ int odp_timer_set_abs(odp_timer_t tim, uint64_t abs_tck, odp_event_t *tmo_ev); Maxim. > > On Mon, Jun 6, 2016 at 2:28 PM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > For some reason we defined function to set timer as int but have enum > in api. It looks more clear to bind return code to current api > description. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > include/odp/api/spec/timer.h | 12 ++++++------ > platform/linux-generic/odp_timer.c | 12 ++++++------ > 2 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/include/odp/api/spec/timer.h > b/include/odp/api/spec/timer.h > index 3f8fdd4..2c3bb8a 100644 > --- a/include/odp/api/spec/timer.h > +++ b/include/odp/api/spec/timer.h > @@ -243,9 +243,9 @@ odp_event_t odp_timer_free(odp_timer_t tim); > * @retval ODP_TIMER_NOEVENT Operation failed because timeout > event not > * specified in odp_timer_set call and not present in timer > */ > -int odp_timer_set_abs(odp_timer_t tim, > - uint64_t abs_tck, > - odp_event_t *tmo_ev); > +odp_timer_set_t odp_timer_set_abs(odp_timer_t tim, > + uint64_t abs_tck, > + odp_event_t *tmo_ev); > > /** > * Set a timer with a relative expiration time and user-provided > event. > @@ -268,9 +268,9 @@ int odp_timer_set_abs(odp_timer_t tim, > * @retval ODP_TIMER_NOEVENT Operation failed because timeout > event not > * specified in call and not present in timer > */ > -int odp_timer_set_rel(odp_timer_t tim, > - uint64_t rel_tck, > - odp_event_t *tmo_ev); > +odp_timer_set_t odp_timer_set_rel(odp_timer_t tim, > + uint64_t rel_tck, > + odp_event_t *tmo_ev); > > /** > * Cancel a timer > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index 996edf0..595b6ab 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -865,9 +865,9 @@ odp_event_t odp_timer_free(odp_timer_t hdl) > return odp_buffer_to_event(old_buf); > } > > -int odp_timer_set_abs(odp_timer_t hdl, > - uint64_t abs_tck, > - odp_event_t *tmo_ev) > +odp_timer_set_t odp_timer_set_abs(odp_timer_t hdl, > + uint64_t abs_tck, > + odp_event_t *tmo_ev) > { > odp_timer_pool *tp = handle_to_tp(hdl); > uint32_t idx = handle_to_idx(hdl, tp); > @@ -882,9 +882,9 @@ int odp_timer_set_abs(odp_timer_t hdl, > return ODP_TIMER_NOEVENT; > } > > -int odp_timer_set_rel(odp_timer_t hdl, > - uint64_t rel_tck, > - odp_event_t *tmo_ev) > +odp_timer_set_t odp_timer_set_rel(odp_timer_t hdl, > + uint64_t rel_tck, > + odp_event_t *tmo_ev) > { > odp_timer_pool *tp = handle_to_tp(hdl); > uint32_t idx = handle_to_idx(hdl, tp); > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
I've added this to the agenda for the Wednesday ARCH call. On Tue, Jun 7, 2016 at 11:43 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 06/06/16 23:16, Bill Fischofer wrote: > >> I disagree with this. The enum in this case is simply an alternative to >> using #defines. These values are in fact ints and ODP API standard practice >> is to return ints, not enums. I don't see any advantage to singling out >> these APIs for different treatment. >> >> One could argue that the "proper" thing to do here would be to return -1 >> and have the specific reason be returned via the odp_errno. But I don't >> think that change is needed either since we define <0 RCs to be error >> returns and its up to each API to decide if it wants to have multiple such >> error return values. >> > Bill, in doxygen description we did not define any <0 RCs, only that we > have in enum. > > So if it's int probably we need to add: > * @retval other != 0 return for other error */ > > > /** > * Set a timer (absolute time) with a user-provided timeout event > * > * Set (arm) the timer to expire at specific time. The timeout > * event will be enqueued when the timer expires. > * > * @param tim Timer > * @param abs_tck Expiration time in absolute timer ticks > * @param[in,out] tmo_ev Reference to an event variable that points to > * timeout event or NULL to reuse the existing timeout event. Any existing > * timeout event that is replaced by a successful set operation will be > * returned here. > * > * @retval ODP_TIMER_SUCCESS Operation succeeded > * @retval ODP_TIMER_TOOEARLY Operation failed because expiration tick too > * early > * @retval ODP_TIMER_TOOLATE Operation failed because expiration tick too > * late > * @retval ODP_TIMER_NOEVENT Operation failed because timeout event not > * specified in odp_timer_set call and not present in timer > */ > int odp_timer_set_abs(odp_timer_t tim, > uint64_t abs_tck, > odp_event_t *tmo_ev); > > > Maxim. > >> >> On Mon, Jun 6, 2016 at 2:28 PM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> For some reason we defined function to set timer as int but have enum >> in api. It looks more clear to bind return code to current api >> description. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> >> --- >> include/odp/api/spec/timer.h | 12 ++++++------ >> platform/linux-generic/odp_timer.c | 12 ++++++------ >> 2 files changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/include/odp/api/spec/timer.h >> b/include/odp/api/spec/timer.h >> index 3f8fdd4..2c3bb8a 100644 >> --- a/include/odp/api/spec/timer.h >> +++ b/include/odp/api/spec/timer.h >> @@ -243,9 +243,9 @@ odp_event_t odp_timer_free(odp_timer_t tim); >> * @retval ODP_TIMER_NOEVENT Operation failed because timeout >> event not >> * specified in odp_timer_set call and not present in timer >> */ >> -int odp_timer_set_abs(odp_timer_t tim, >> - uint64_t abs_tck, >> - odp_event_t *tmo_ev); >> +odp_timer_set_t odp_timer_set_abs(odp_timer_t tim, >> + uint64_t abs_tck, >> + odp_event_t *tmo_ev); >> >> /** >> * Set a timer with a relative expiration time and user-provided >> event. >> @@ -268,9 +268,9 @@ int odp_timer_set_abs(odp_timer_t tim, >> * @retval ODP_TIMER_NOEVENT Operation failed because timeout >> event not >> * specified in call and not present in timer >> */ >> -int odp_timer_set_rel(odp_timer_t tim, >> - uint64_t rel_tck, >> - odp_event_t *tmo_ev); >> +odp_timer_set_t odp_timer_set_rel(odp_timer_t tim, >> + uint64_t rel_tck, >> + odp_event_t *tmo_ev); >> >> /** >> * Cancel a timer >> diff --git a/platform/linux-generic/odp_timer.c >> b/platform/linux-generic/odp_timer.c >> index 996edf0..595b6ab 100644 >> --- a/platform/linux-generic/odp_timer.c >> +++ b/platform/linux-generic/odp_timer.c >> @@ -865,9 +865,9 @@ odp_event_t odp_timer_free(odp_timer_t hdl) >> return odp_buffer_to_event(old_buf); >> } >> >> -int odp_timer_set_abs(odp_timer_t hdl, >> - uint64_t abs_tck, >> - odp_event_t *tmo_ev) >> +odp_timer_set_t odp_timer_set_abs(odp_timer_t hdl, >> + uint64_t abs_tck, >> + odp_event_t *tmo_ev) >> { >> odp_timer_pool *tp = handle_to_tp(hdl); >> uint32_t idx = handle_to_idx(hdl, tp); >> @@ -882,9 +882,9 @@ int odp_timer_set_abs(odp_timer_t hdl, >> return ODP_TIMER_NOEVENT; >> } >> >> -int odp_timer_set_rel(odp_timer_t hdl, >> - uint64_t rel_tck, >> - odp_event_t *tmo_ev) >> +odp_timer_set_t odp_timer_set_rel(odp_timer_t hdl, >> + uint64_t rel_tck, >> + odp_event_t *tmo_ev) >> { >> odp_timer_pool *tp = handle_to_tp(hdl); >> uint32_t idx = handle_to_idx(hdl, tp); >> -- >> 2.7.1.250.gff4ea60 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h index 3f8fdd4..2c3bb8a 100644 --- a/include/odp/api/spec/timer.h +++ b/include/odp/api/spec/timer.h @@ -243,9 +243,9 @@ odp_event_t odp_timer_free(odp_timer_t tim); * @retval ODP_TIMER_NOEVENT Operation failed because timeout event not * specified in odp_timer_set call and not present in timer */ -int odp_timer_set_abs(odp_timer_t tim, - uint64_t abs_tck, - odp_event_t *tmo_ev); +odp_timer_set_t odp_timer_set_abs(odp_timer_t tim, + uint64_t abs_tck, + odp_event_t *tmo_ev); /** * Set a timer with a relative expiration time and user-provided event. @@ -268,9 +268,9 @@ int odp_timer_set_abs(odp_timer_t tim, * @retval ODP_TIMER_NOEVENT Operation failed because timeout event not * specified in call and not present in timer */ -int odp_timer_set_rel(odp_timer_t tim, - uint64_t rel_tck, - odp_event_t *tmo_ev); +odp_timer_set_t odp_timer_set_rel(odp_timer_t tim, + uint64_t rel_tck, + odp_event_t *tmo_ev); /** * Cancel a timer diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c index 996edf0..595b6ab 100644 --- a/platform/linux-generic/odp_timer.c +++ b/platform/linux-generic/odp_timer.c @@ -865,9 +865,9 @@ odp_event_t odp_timer_free(odp_timer_t hdl) return odp_buffer_to_event(old_buf); } -int odp_timer_set_abs(odp_timer_t hdl, - uint64_t abs_tck, - odp_event_t *tmo_ev) +odp_timer_set_t odp_timer_set_abs(odp_timer_t hdl, + uint64_t abs_tck, + odp_event_t *tmo_ev) { odp_timer_pool *tp = handle_to_tp(hdl); uint32_t idx = handle_to_idx(hdl, tp); @@ -882,9 +882,9 @@ int odp_timer_set_abs(odp_timer_t hdl, return ODP_TIMER_NOEVENT; } -int odp_timer_set_rel(odp_timer_t hdl, - uint64_t rel_tck, - odp_event_t *tmo_ev) +odp_timer_set_t odp_timer_set_rel(odp_timer_t hdl, + uint64_t rel_tck, + odp_event_t *tmo_ev) { odp_timer_pool *tp = handle_to_tp(hdl); uint32_t idx = handle_to_idx(hdl, tp);
For some reason we defined function to set timer as int but have enum in api. It looks more clear to bind return code to current api description. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- include/odp/api/spec/timer.h | 12 ++++++------ platform/linux-generic/odp_timer.c | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-)