Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Monday, July 6, 2020 12:19 AM
> To: jer...@marvell.com; Ori Kam <or...@mellanox.com>
> Cc: xiang.w.w...@intel.com; dev@dpdk.org; g...@marvell.com;
> pbhagavat...@marvell.com; Shahaf Shuler <shah...@mellanox.com>;
> hemant.agra...@nxp.com; Opher Reviv <op...@mellanox.com>; Alex
> Rosenbaum <al...@mellanox.com>; dov...@marvell.com;
> pkap...@marvell.com; nipun.gu...@nxp.com; bruce.richard...@intel.com;
> yang.a.h...@intel.com; harry.ch...@intel.com; gu.ji...@zte.com.cn;
> shanjia...@chinatelecom.cn; zhangy....@chinatelecom.cn;
> lixin...@huachentel.com; wush...@inspur.com; yuying...@yxlink.com;
> fanchengg...@sunyainfo.com; davidf...@tencent.com;
> liuzho...@chinaunicom.cn; zhaoyon...@huawei.com; o...@yunify.com;
> j...@netgate.com; hongjun...@intel.com; d...@ntop.org; f...@napatech.com;
> arthur...@lionic.com
> Subject: Re: [dpdk-dev] [PATCH v4 1/4] regexdev: introduce regexdev
> subsystem
>
> 02/07/2020 09:46, Ori Kam:
> > From: Jerin Jacob <jer...@marvell.com>
> > --- a/config/common_base
> > +++ b/config/common_base
> > #
> > +# Compile regex device support
> > +#
> > +CONFIG_RTE_LIBRTE_REGEXDEV=y
> > +
> > +#
> > # Compile librte_ring
> > #
> > CONFIG_RTE_LIBRTE_RING=y
> > @@ -1141,3 +1146,4 @@ CONFIG_RTE_APP_CRYPTO_PERF=y
> > # Compile the eventdev application
> > #
> > CONFIG_RTE_APP_EVENTDEV=y
> > +
>
> Why this empty line?
>
Sure.
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -26,6 +26,7 @@ The public API headers are grouped by topics:
> > [event_timer_adapter] (@ref rte_event_timer_adapter.h),
> > [event_crypto_adapter] (@ref rte_event_crypto_adapter.h),
> > [rawdev] (@ref rte_rawdev.h),
> > + [regexdev] (@ref rte_regexdev.h),
> > [metrics] (@ref rte_metrics.h),
>
> Please move regexdev after [compress].
>
Sure.
> > --- a/doc/api/doxy-api.conf.in
> > +++ b/doc/api/doxy-api.conf.in
> > @@ -61,6 +61,7 @@ INPUT = @TOPDIR@/doc/api/doxy-api-
> index.md \
> > @TOPDIR@/lib/librte_rcu \
> > @TOPDIR@/lib/librte_reorder \
> > @TOPDIR@/lib/librte_rib \
> > + @TOPDIR@/lib/librte_regexdev \
> > @TOPDIR@/lib/librte_ring \
>
> What is the ordering here?
>
Will move it above rib.
> > --- a/doc/guides/prog_guide/index.rst
> > +++ b/doc/guides/prog_guide/index.rst
> > @@ -72,3 +72,4 @@ Programmer's Guide
> > lto
> > profile_app
> > glossary
> > + regexdev_lib
>
> Why adding it at the end?
> I would suggest adding regexdev after compressdev.
>
Sure.
> > --- /dev/null
> > +++ b/doc/guides/prog_guide/regexdev_lib.rst
>
> Please remove the _lib suffix in this filename.
>
Sure.
> [...]
> > +The dequeue API uses the same format as the enqueue API of processed but
> > +the ``nb_ops`` and ``ops`` parameters are now used to specify the max
> processed
> > +operations the user wishes to retrieve and the location in which to store
> them.
> > +The API call returns the actual number of processed operations returned,
> this
> > +can never be larger than ``nb_ops``.
> > +
>
> Please avoid empty line at end of files.
>
Sure.
> > --- /dev/null
> > +++ b/lib/librte_regexdev/Makefile
> > @@ -0,0 +1,31 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(C) 2019 Marvell International Ltd.
> > +# Copyright(C) 2020 Mellanox International Ltd.
>
> Mellanox copyright is wrong. It should be:
> Copyright 2020 Mellanox Technologies, Ltd
>
Will fix.
> > --- /dev/null
> > +++ b/lib/librte_regexdev/meson.build
> > @@ -0,0 +1,7 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2020 Mellanox Corporation
>
> Wrong copyright. Please check them all.
>
Will fix.
> > +
> > +allow_experimental_apis = true
>
> Internal libraries do not need such flag anymore.
>
Will remove.
> > +sources = files('rte_regexdev.c')
> > +headers = files('rte_regexdev.h')
> > +deps += ['mbuf']
> [...]
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -25,7 +25,7 @@ libraries = [
> > 'gro', 'gso', 'ip_frag', 'jobstats',
> > 'kni', 'latencystats', 'lpm', 'member',
> > 'power', 'pdump', 'rawdev',
> > - 'rib', 'reorder', 'sched', 'security', 'stack', 'vhost',
> > + 'regexdev', 'rib', 'reorder', 'sched', 'security', 'stack', 'vhost',
>
> strange choice :)
> I would have added regex at the end of the previous line which is shorter.
>
Will change.
>
