diff mbox series

[RFC,4/7] telemetry: make array initialization more robust

Message ID	20221213182730.97065-5-bruce.richardson@intel.com (mailing list archive)
State	Superseded, archived
Delegated to:	Thomas Monjalon
Headers	From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: Bruce Richardson <bruce.richardson@intel.com> Subject: [RFC PATCH 4/7] telemetry: make array initialization more robust Date: Tue, 13 Dec 2022 18:27:27 +0000 Message-Id: <20221213182730.97065-5-bruce.richardson@intel.com> In-Reply-To: <20221213182730.97065-1-bruce.richardson@intel.com> References: <20221213182730.97065-1-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: dev-bounces@dpdk.org
Series	Standardize telemetry int types \| [RFC,0/7] Standardize telemetry int types [RFC,1/7] telemetry: rename unsigned 64-bit enum value to uint [RFC,2/7] telemetry: add uint type as alias for u64 [RFC,3/7] telemetry: remove RTE prefix from internal enum values [RFC,4/7] telemetry: make array initialization more robust [RFC,5/7] telemetry: update json functions to use int/uint in names [RFC,6/7] telemetry: make internal int representation 64-bits [RFC,7/7] telemetry: change public API to use 64-bit signed values

Checks

Context	Check	Description
ci/checkpatch	success	coding style OK

Commit Message

Bruce Richardson Dec. 13, 2022, 6:27 p.m. UTC

  Rather than relying on a specific ordering of elements in the array
matching that of elements in the enum definition, we can explicitly mark
each array entry using the equivalent enum value as an index.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_data.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Tyler Retzlaff Dec. 14, 2022, 5:50 p.m. UTC | #1

On Tue, Dec 13, 2022 at 06:27:27PM +0000, Bruce Richardson wrote:
> Rather than relying on a specific ordering of elements in the array
> matching that of elements in the enum definition, we can explicitly mark
> each array entry using the equivalent enum value as an index.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/telemetry/telemetry_data.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index d51724e1f5..9a180937fd 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -16,10 +16,10 @@ int
>  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
>  {
>  	enum tel_container_types array_types[] = {
> -			TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> -			TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> -			TEL_ARRAY_UINT,    /* RTE_TEL_UINT_VAL = 2 */
> -			TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> +			[RTE_TEL_STRING_VAL] = TEL_ARRAY_STRING,
> +			[RTE_TEL_INT_VAL] = TEL_ARRAY_INT,
> +			[RTE_TEL_UINT_VAL] = TEL_ARRAY_UINT,
> +			[RTE_TEL_CONTAINER] = TEL_ARRAY_CONTAINER,
>  	};

i might be a bit fuzzy and didn't double check but doesn't doing this
require C99?

though it would be great to move to a minimum of C99/C11

>  	d->type = array_types[type];
>  	d->data_len = 0;
> -- 
> 2.34.1

Bruce Richardson Jan. 9, 2023, 12:16 p.m. UTC | #2

On Wed, Dec 14, 2022 at 09:50:33AM -0800, Tyler Retzlaff wrote:
> On Tue, Dec 13, 2022 at 06:27:27PM +0000, Bruce Richardson wrote:
> > Rather than relying on a specific ordering of elements in the array
> > matching that of elements in the enum definition, we can explicitly mark
> > each array entry using the equivalent enum value as an index.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/telemetry/telemetry_data.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index d51724e1f5..9a180937fd 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -16,10 +16,10 @@ int
> >  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> >  {
> >  	enum tel_container_types array_types[] = {
> > -			TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> > -			TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> > -			TEL_ARRAY_UINT,    /* RTE_TEL_UINT_VAL = 2 */
> > -			TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> > +			[RTE_TEL_STRING_VAL] = TEL_ARRAY_STRING,
> > +			[RTE_TEL_INT_VAL] = TEL_ARRAY_INT,
> > +			[RTE_TEL_UINT_VAL] = TEL_ARRAY_UINT,
> > +			[RTE_TEL_CONTAINER] = TEL_ARRAY_CONTAINER,
> >  	};
> 
> i might be a bit fuzzy and didn't double check but doesn't doing this
> require C99?
> 
> though it would be great to move to a minimum of C99/C11
> 
Yep, I agree on version bump.

For the specific array init - we actually already use this style of init
elsewhere in telemetry lib, so I'm going to keep it here in V2, as I
think it is the clearest way to initialize a lookup array like this.

/Bruce

Tyler Retzlaff Jan. 9, 2023, 5:49 p.m. UTC | #3

On Mon, Jan 09, 2023 at 12:16:15PM +0000, Bruce Richardson wrote:
> On Wed, Dec 14, 2022 at 09:50:33AM -0800, Tyler Retzlaff wrote:
> > On Tue, Dec 13, 2022 at 06:27:27PM +0000, Bruce Richardson wrote:
> > > Rather than relying on a specific ordering of elements in the array
> > > matching that of elements in the enum definition, we can explicitly mark
> > > each array entry using the equivalent enum value as an index.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  lib/telemetry/telemetry_data.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index d51724e1f5..9a180937fd 100644
> > > --- a/lib/telemetry/telemetry_data.c
> > > +++ b/lib/telemetry/telemetry_data.c
> > > @@ -16,10 +16,10 @@ int
> > >  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> > >  {
> > >  	enum tel_container_types array_types[] = {
> > > -			TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> > > -			TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> > > -			TEL_ARRAY_UINT,    /* RTE_TEL_UINT_VAL = 2 */
> > > -			TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> > > +			[RTE_TEL_STRING_VAL] = TEL_ARRAY_STRING,
> > > +			[RTE_TEL_INT_VAL] = TEL_ARRAY_INT,
> > > +			[RTE_TEL_UINT_VAL] = TEL_ARRAY_UINT,
> > > +			[RTE_TEL_CONTAINER] = TEL_ARRAY_CONTAINER,
> > >  	};
> > 
> > i might be a bit fuzzy and didn't double check but doesn't doing this
> > require C99?
> > 
> > though it would be great to move to a minimum of C99/C11
> > 
> Yep, I agree on version bump.
> 
> For the specific array init - we actually already use this style of init
> elsewhere in telemetry lib, so I'm going to keep it here in V2, as I
> think it is the clearest way to initialize a lookup array like this.

sounds good given our other discussion about moving to C99 as a minimum.

> 
> /Bruce

Ferruh Yigit Jan. 10, 2023, 9:11 a.m. UTC | #4

On 1/9/2023 5:49 PM, Tyler Retzlaff wrote:
> On Mon, Jan 09, 2023 at 12:16:15PM +0000, Bruce Richardson wrote:
>> On Wed, Dec 14, 2022 at 09:50:33AM -0800, Tyler Retzlaff wrote:
>>> On Tue, Dec 13, 2022 at 06:27:27PM +0000, Bruce Richardson wrote:
>>>> Rather than relying on a specific ordering of elements in the array
>>>> matching that of elements in the enum definition, we can explicitly mark
>>>> each array entry using the equivalent enum value as an index.
>>>>
>>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>> ---
>>>>  lib/telemetry/telemetry_data.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
>>>> index d51724e1f5..9a180937fd 100644
>>>> --- a/lib/telemetry/telemetry_data.c
>>>> +++ b/lib/telemetry/telemetry_data.c
>>>> @@ -16,10 +16,10 @@ int
>>>>  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
>>>>  {
>>>>  	enum tel_container_types array_types[] = {
>>>> -			TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
>>>> -			TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
>>>> -			TEL_ARRAY_UINT,    /* RTE_TEL_UINT_VAL = 2 */
>>>> -			TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
>>>> +			[RTE_TEL_STRING_VAL] = TEL_ARRAY_STRING,
>>>> +			[RTE_TEL_INT_VAL] = TEL_ARRAY_INT,
>>>> +			[RTE_TEL_UINT_VAL] = TEL_ARRAY_UINT,
>>>> +			[RTE_TEL_CONTAINER] = TEL_ARRAY_CONTAINER,
>>>>  	};
>>>
>>> i might be a bit fuzzy and didn't double check but doesn't doing this
>>> require C99?
>>>
>>> though it would be great to move to a minimum of C99/C11
>>>
>> Yep, I agree on version bump.
>>
>> For the specific array init - we actually already use this style of init
>> elsewhere in telemetry lib, so I'm going to keep it here in V2, as I
>> think it is the clearest way to initialize a lookup array like this.
> 
> sounds good given our other discussion about moving to C99 as a minimum.
> 

There were some drivers requiring C89/90 but as far as I know all
updated at this stage,

+1 to move C99 support.

diff mbox series

Patch

diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index d51724e1f5..9a180937fd 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -16,10 +16,10 @@  int
 rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
 {
 	enum tel_container_types array_types[] = {
-			TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
-			TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
-			TEL_ARRAY_UINT,    /* RTE_TEL_UINT_VAL = 2 */
-			TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
+			[RTE_TEL_STRING_VAL] = TEL_ARRAY_STRING,
+			[RTE_TEL_INT_VAL] = TEL_ARRAY_INT,
+			[RTE_TEL_UINT_VAL] = TEL_ARRAY_UINT,
+			[RTE_TEL_CONTAINER] = TEL_ARRAY_CONTAINER,
 	};
 	d->type = array_types[type];
 	d->data_len = 0;