Hi Thomas,
I will update all your comments below today, thanks.
The one where we need your confirmation is specifically this comment from your,
I believe we discussed but good to make sure we are all aligned:
> But the big question is why do we need this "MAX" value?
> The guideline is to avoid using such MAX value for long term compatibility.
This is not a _MAX enum but a _SIZE_MAX for array related to that enum. Note
that the actual max value of the enum exists but is used a private macro.
The distinction is that the application cannot make any assumptions on what is
the maximum enum value (ie. we don't want enum with MAX value, that is not
future proof has captured in doc).
But the application can make some assumption on the sizing of array based on
such an enum. The difference being the padding which allows for the enum growth
without breaking ABI or application.
The previous name was _PADDED_MAX to make it clear this not a max enum but a
padded value. Then more recenrtly the consensus in the community was to change
this to _SIZE_MAX to be arguably more explicit this is to be used for array
size. The comments I believe also make it clear this is not a MAX enum.
Does that make sense and do you agree this is best consensus so far to move
forward?
Thanks Thomas,
Nic
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Monday, October 3, 2022 1:29 AM
> To: Chautru, Nicolas <nicolas.chau...@intel.com>
> Cc: dev@dpdk.org; gak...@marvell.com; maxime.coque...@redhat.com;
> t...@redhat.com; m...@ashroe.eu; Richardson, Bruce
> <bruce.richard...@intel.com>; david.march...@redhat.com;
> step...@networkplumber.org; Zhang, Mingshan
> <mingshan.zh...@intel.com>; hemant.agra...@nxp.com
> Subject: Re: [PATCH v10 6/7] bbdev: add queue related warning and status
> information
>
> Looking at this patch because I have been alerted about the ABI compat
> handling.
> I see some details that should have been caught in earlier reviews.
>
> 30/09/2022 20:46, Nicolas Chautru:
>
> > +/*
> > + * Maximum size to be used to manage the enum
> > +rte_bbdev_enqueue_status including padding for future
>
> This line is long.
> It is always better to split lines logically, for instance here, before
> "including".
>
> > + * enum insertion
>
> It could be made clear that the real enum size is smaller or equal.
>
> > + */
> > +#define RTE_BBDEV_ENQ_STATUS_SIZE_MAX 6
> [...]
> > +enum rte_bbdev_enqueue_status {
> > + RTE_BBDEV_ENQ_STATUS_NONE, /**< Nothing to report */
> > + RTE_BBDEV_ENQ_STATUS_QUEUE_FULL, /**< Not enough room in
> queue */
> > + RTE_BBDEV_ENQ_STATUS_RING_FULL, /**< Not enough room in
> ring */
> > + RTE_BBDEV_ENQ_STATUS_INVALID_OP, /**< Operation was
> rejected as invalid */
> > +};
>
> A comment is missing at the end of the enum to remind updating the MAX.
>
> But the big question is why do we need this "MAX" value?
> The guideline is to avoid using such MAX value for long term compatibility.
>
> [...]
> > +/**
> > + * Converts queue status from enum to string
>
> Should be imperative form: "Convert".
> A dot is missing at the end of the sentence.
>
> > + *
> > + * @param status
> > + * Queue status as enum
> > + *
> > + * @returns
> > + * Queue status as string or NULL if op_type is invalid
>
> It is not aligned with above parameter.
> Choose an indentation format and keep it consistent.
>
> [...]
> > # added in 22.11
> > rte_bbdev_device_status_str;
> > + rte_bbdev_enqueue_status_str;
> > rte_bbdev_enqueue_fft_ops;
> > rte_bbdev_dequeue_fft_ops;
>
> It is not alphabetical order.
>
>
