[API-NEXT] api: timer: return code of set timer to enum

Message ID 1465241288-5308-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov June 6, 2016, 7:28 p.m.
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(-)

Comments

Bill Fischofer June 6, 2016, 8:16 p.m. | #1
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
>
Maxim Uvarov June 7, 2016, 4:43 p.m. | #2
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
>
>
Bill Fischofer June 8, 2016, 3:08 a.m. | #3
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
>>
>>
>>
>

Patch

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);