Re: [dpdk-dev] [PATCH v3 01/29] graph: define the public API for graph support

[Jerin Jacob <jerinjacobk@gmail.com>]

On Mon, Apr 6, 2020 at 9:39 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>
> On 4/6/20 4:59 PM, Jerin Jacob wrote:
> > On Mon, Apr 6, 2020 at 6:06 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
> [...]
> >>> +typedef uint32_t rte_graph_off_t;  /**< Graph offset type. */
> >>> +typedef uint32_t rte_node_t;       /**< Node id type. */
> >>> +typedef uint16_t rte_edge_t;       /**< Edge id type. */
> >>> +typedef uint16_t rte_graph_t;      /**< Graph id type. */
> >>
> >> I would use 'id' somewhere in the name of these typedefs - e.g. seeing
> >> rte_node_t in the code (without knowing what it is) I'd be guessing this
> >> is a pointer to 'struct rte_node'.
> >> So maybe 'rte_node_id' or if we stick with _t convention and
> >> rte_node_id_t is too long then maybe simple rte_nid_t/rte_eid_t/rte_gid_t?
> >
> > Considering typedef will not be pointers in Linux coding standard, I
> > have chosen shorter
> > name. considering eid, gid is cryptic and Since you think, rte_node_id
> > better, I will change to
> > that.
>
> If the typedef are not pointers by the coding standard then I'm fine
> with current names - no need to change.

Ack.

>
> [...]
> >> [...]
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice
> >>> + *
> >>> + * Get the number of edges for a node from node id.
> >>> + *
> >>> + * @param id
> >>> + *   Valid node id.
> >>> + *
> >>> + * @return
> >>> + *   Valid edge count on success, RTE_EDGE_ID_INVALID otherwise.
> >>> + */
> >>> +__rte_experimental
> >>> +rte_edge_t rte_node_edge_count(rte_node_t id);
> >>
> >> I would clarify here what edge is?  Incoming nodes, next-nodes or both.
> >
> > It is next-node. I will update the doc.
> >
> >>  Why edge-id typedef on return and why EDGE_ID_INVALID returned.  Would
> >> int/-EINVAL (for wrong 'id') be better?
> >
> > Edge node ID is expressed in rte_edge_id_t. SO, I think, it fine to return
> > rte_edge_id_t. "This would avoid any compassion issue as well."
>
> Did not understand the last sentence.  Could you rephrase it?

Sorry. I meant signed vs unsigned comparison if needed in the
variable. ie if (int < uint32_t)

>
> With regards
> Andrzej Ostruszka