On 2 June 2015 at 11:22, Stuart Haslam <[email protected]> wrote:
> On Thu, May 28, 2015 at 12:58:08PM +0200, Christophe Milard wrote: > > Renaming of things which may be, one day, exported in a lib. > > This renaming is important, as it creates consistency between test > > symbols, which is needed if things get eventually exported in the lib. > > Also, tests are often created from other tests: Fixing the first exemples > > will help geting future tests better. > > > > Also defining local test main. > > > > Things that are candidate to be exported in the lib in the future > > have been named as follows: > > -Tests, i.e. functions which are used in CUNIT testsuites are named: > > <Module>_test_* > > -Test arrays, i.e. arrays of CU_TestInfo, listing the test functions > > belonging to a suite, are called: > > <Module>_suite[_*] > > where the possible suffix can be used if many suites are declared. > > -CUNIT suite init and termination functions are called: > > <Module>_suite[_*]_init() and <Module>_suite[_*]_term() > > respectively. > > -Suite arrays, i.e. arrays of CU_SuiteInfo used in executables are > called: > > <Module>_suites[_*] > > where the possible suffix identifies the executable using it, if many. > > -Main function(s), are called: > > <Module>_main[_*] > > where the possible suffix identifies the executable using it > > > > Signed-off-by: Christophe Milard <[email protected]> > > --- > > test/Makefile.inc | 7 +++- > > test/validation/Makefile.am | 8 ++--- > > test/validation/odp_pktio.c | 81 > +++++++++++++++++++++++++-------------------- > > 3 files changed, 55 insertions(+), 41 deletions(-) > > > > diff --git a/test/Makefile.inc b/test/Makefile.inc > > index e20a848..c579c46 100644 > > --- a/test/Makefile.inc > > +++ b/test/Makefile.inc > > @@ -1,7 +1,12 @@ > > include $(top_srcdir)/Makefile.inc > > include $(top_srcdir)/platform/@with_platform@/Makefile.inc > > LIB = $(top_builddir)/lib > > -LDADD = $(LIB)/libodp.la > > + > > +#in the following line, the libs using the symbols should come before > > +#the libs using them! The includer is given a chance to add things > before libodp > > +#by setting PRE_LDDAD before the inclusion. > > +LDADD = $(PRE_LDDAD) $(LIB)/libodp.la > > + > > INCFLAGS = -I$(srcdir) \ > > -I$(top_srcdir)/test \ > > -I$(top_srcdir)/platform/@with_platform@/include \ > > diff --git a/test/validation/Makefile.am b/test/validation/Makefile.am > > index 965f603..8299e01 100644 > > --- a/test/validation/Makefile.am > > +++ b/test/validation/Makefile.am > > @@ -1,13 +1,9 @@ > > +PRE_LDDAD = > $(top_builddir)/test/validation/common/libcunit_common_as_main.a > > include $(top_srcdir)/test/Makefile.inc > > Shouldn't this be named PRE_LDADD? > Yes, of course. will be fix in V2 > > > > > AM_CFLAGS += -I$(srcdir)/common > > AM_LDFLAGS += -static > > > > -#warning: in the following line, the libs using the symbols should come > before > > -#the libs using them! > > -LDADD = > $(top_builddir)/test/validation/common/libcunit_common_as_main.a \ > > - $(LIB)/libodp.la > > - > > TESTS_ENVIRONMENT = ODP_PLATFORM=${with_platform} TEST_DIR=${builddir} > > > > EXECUTABLES = odp_buffer \ > > @@ -62,6 +58,8 @@ dist_odp_shared_memory_SOURCES = > odp_shared_memory.c > > dist_odp_synchronizers_SOURCES = odp_synchronizers.c > > dist_odp_time_SOURCES = odp_time.c > > dist_odp_timer_SOURCES = odp_timer.c > > +odp_pktio_LDADD = > $(top_builddir)/test/validation/common/libcunit_common.a \ > > + $(LIB)/libodp.la > > dist_odp_pktio_SOURCES = odp_pktio.c > > dist_odp_packet_SOURCES = odp_packet.c > > dist_odp_pool_SOURCES = odp_pool.c > > diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c > > index d4bf7bf..9806376 100644 > > --- a/test/validation/odp_pktio.c > > +++ b/test/validation/odp_pktio.c > > @@ -420,7 +420,7 @@ static void pktio_txrx_multi(pktio_info_t *pktio_a, > pktio_info_t *pktio_b, > > CU_ASSERT(i == num_pkts); > > } > > > > -static void pktio_test_txrx(odp_queue_type_t q_type, int num_pkts) > > +static void test_txrx(odp_queue_type_t q_type, int num_pkts) > > { > > int ret, i, if_b; > > pktio_info_t pktios[MAX_NUM_IFACES]; > > @@ -456,34 +456,34 @@ static void pktio_test_txrx(odp_queue_type_t > q_type, int num_pkts) > > } > > } > > > > -static void test_odp_pktio_poll_queue(void) > > +static void pktio_test_poll_queue(void) > > { > > - pktio_test_txrx(ODP_QUEUE_TYPE_POLL, 1); > > + test_txrx(ODP_QUEUE_TYPE_POLL, 1); > > } > > > > -static void test_odp_pktio_poll_multi(void) > > +static void pktio_test_poll_multi(void) > > { > > - pktio_test_txrx(ODP_QUEUE_TYPE_POLL, 4); > > + test_txrx(ODP_QUEUE_TYPE_POLL, 4); > > } > > > > -static void test_odp_pktio_sched_queue(void) > > +static void pktio_test_sched_queue(void) > > { > > - pktio_test_txrx(ODP_QUEUE_TYPE_SCHED, 1); > > + test_txrx(ODP_QUEUE_TYPE_SCHED, 1); > > } > > > > -static void test_odp_pktio_sched_multi(void) > > +static void pktio_test_sched_multi(void) > > { > > - pktio_test_txrx(ODP_QUEUE_TYPE_SCHED, 4); > > + test_txrx(ODP_QUEUE_TYPE_SCHED, 4); > > } > > > > -static void test_odp_pktio_jumbo(void) > > +static void pktio_test_jumbo(void) > > { > > packet_len = PKT_LEN_JUMBO; > > - test_odp_pktio_sched_multi(); > > + pktio_test_sched_multi(); > > packet_len = PKT_LEN_NORMAL; > > } > > > > -static void test_odp_pktio_mtu(void) > > +static void pktio_test_mtu(void) > > { > > int ret; > > int mtu; > > @@ -500,7 +500,7 @@ static void test_odp_pktio_mtu(void) > > return; > > } > > > > -static void test_odp_pktio_promisc(void) > > +static void pktio_test_promisc(void) > > { > > int ret; > > odp_pktio_t pktio = create_pktio(iface_name[0]); > > @@ -525,7 +525,7 @@ static void test_odp_pktio_promisc(void) > > return; > > } > > > > -static void test_odp_pktio_mac(void) > > +static void pktio_test_mac(void) > > { > > unsigned char mac_addr[ODPH_ETHADDR_LEN]; > > int mac_len; > > @@ -551,7 +551,7 @@ static void test_odp_pktio_mac(void) > > return; > > } > > > > -static void test_odp_pktio_inq_remdef(void) > > +static void pktio_test_inq_remdef(void) > > { > > odp_pktio_t pktio = create_pktio(iface_name[0]); > > odp_queue_t inq; > > @@ -575,7 +575,7 @@ static void test_odp_pktio_inq_remdef(void) > > CU_ASSERT(odp_pktio_close(pktio) == 0); > > } > > > > -static void test_odp_pktio_open(void) > > +static void pktio_test_open(void) > > { > > odp_pktio_t pktio; > > int i; > > @@ -591,7 +591,7 @@ static void test_odp_pktio_open(void) > > CU_ASSERT(pktio == ODP_PKTIO_INVALID); > > } > > > > -static void test_odp_pktio_lookup(void) > > +static void pktio_test_lookup(void) > > { > > odp_pktio_t pktio, pktio_inval; > > > > @@ -609,7 +609,7 @@ static void test_odp_pktio_lookup(void) > > CU_ASSERT(odp_pktio_lookup(iface_name[0]) == ODP_PKTIO_INVALID); > > } > > > > -static void test_odp_pktio_inq(void) > > +static void pktio_test_inq(void) > > { > > odp_pktio_t pktio; > > > > @@ -621,7 +621,7 @@ static void test_odp_pktio_inq(void) > > CU_ASSERT(odp_pktio_close(pktio) == 0); > > } > > > > -static int init_pktio_suite(void) > > +static int pktio_suite_init(void) > > { > > odp_atomic_init_u32(&ip_seq, 0); > > iface_name[0] = getenv("ODP_PKTIO_IF0"); > > @@ -647,7 +647,7 @@ static int init_pktio_suite(void) > > return 0; > > } > > > > -static int term_pktio_suite(void) > > +static int pktio_suite_term(void) > > { > > char pool_name[ODP_POOL_NAME_LEN]; > > odp_pool_t pool; > > @@ -676,24 +676,35 @@ static int term_pktio_suite(void) > > return ret; > > } > > Are all of these renames really necessary given that these symbols are > all still static? We use static so that we don't have to worry about > global namespace pollution and naming decisions are local. This waypoint > of naming things to account for the fact they may one day be exported > but leaving them static isn't natural and will be hard to enforce as > nothing actually breaks if you don't do it and we have no tool that > checks for it. I think it would be better to defer the renaming until > it's actually needed. > > I am not sure I agree with this: As the number of tests increases, the number of things to (possibly) be fixed in the future will increase. Trying to set a naming convention now will just make things more consistent, and possibly reduce the work in the future. At worse, it is just a bit better :-). > > > > -CU_TestInfo pktio_tests[] = { > > - {"pktio open", test_odp_pktio_open}, > > - {"pktio lookup", test_odp_pktio_lookup}, > > - {"pktio inq", test_odp_pktio_inq}, > > - {"pktio poll queues", test_odp_pktio_poll_queue}, > > - {"pktio poll multi", test_odp_pktio_poll_multi}, > > - {"pktio sched queues", test_odp_pktio_sched_queue}, > > - {"pktio sched multi", test_odp_pktio_sched_multi}, > > - {"pktio jumbo frames", test_odp_pktio_jumbo}, > > - {"pktio mtu", test_odp_pktio_mtu}, > > - {"pktio promisc mode", test_odp_pktio_promisc}, > > - {"pktio mac", test_odp_pktio_mac}, > > - {"pktio inq_remdef", test_odp_pktio_inq_remdef}, > > +CU_TestInfo pktio_suite[] = { > > + {"pktio open", pktio_test_open}, > > + {"pktio lookup", pktio_test_lookup}, > > + {"pktio inq", pktio_test_inq}, > > + {"pktio poll queues", pktio_test_poll_queue}, > > + {"pktio poll multi", pktio_test_poll_multi}, > > + {"pktio sched queues", pktio_test_sched_queue}, > > + {"pktio sched multi", pktio_test_sched_multi}, > > + {"pktio jumbo frames", pktio_test_jumbo}, > > + {"pktio mtu", pktio_test_mtu}, > > + {"pktio promisc mode", pktio_test_promisc}, > > + {"pktio mac", pktio_test_mac}, > > + {"pktio inq_remdef", pktio_test_inq_remdef}, > > CU_TEST_INFO_NULL > > }; > > > > -CU_SuiteInfo odp_testsuites[] = { > > +static CU_SuiteInfo pktio_suites[] = { > > {"Packet I/O", > > - init_pktio_suite, term_pktio_suite, NULL, NULL, > pktio_tests}, > > + pktio_suite_init, pktio_suite_term, NULL, NULL, > pktio_suite}, > > CU_SUITE_INFO_NULL > > }; > > Any reason why pktio_suites is static and pktio_suite isn't? > no. Thanks. will be fixed in V2 Thanks, Christophe. > > > + > > +static int pktio_main(void) > > +{ > > + return odp_cunit_run(pktio_suites); > > +} > > + > > +/* the following main function will be separated when lib is created */ > > +int main(void) > > +{ > > + return pktio_main(); > > +} > > -- > > 1.9.1 > > > > -- > Stuart. >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
