[dpdk-dev] [PATCH v1 01/10] app/test: introduce resources for tests

[Jan Viktorin]

On Fri, 06 May 2016 16:01:08 +0200
Thomas Monjalon <thomas.monjalon at 6wind.com> wrote:

> 2016-05-06 12:48, Jan Viktorin:
> > --- /dev/null
> > +++ b/app/test/resource.h
> > @@ -0,0 +1,61 @@
> > +/*-
> > + *   BSD LICENSE  
> [...]
> > + */  
> 
> Please include a multi-line comment here to explain what is a resource
> and why it is needed.

Will fix.

> 
> > +
> > +#ifndef _RESOURCE_H_
> > +#define _RESOURCE_H_
> > +  
> |...]
> |> +
> > +TAILQ_HEAD(resource_list, resource);
> > +extern struct resource_list resource_list;  
> 
> Why extern?

Will fix.

> 
> > +
> > +struct resource {
> > +	const char *name;
> > +	const char *beg;  
> 
> begin?
> Bruce is removing some megabytes from lpm tests, so we have
> some room for field names ;)

I love 3-chars identifiers... ;)

I will rework this, thanks.

> 
> > +	const char *end;
> > +	TAILQ_ENTRY(resource) next;
> > +};
> > +
> > +static inline size_t resource_size(const struct resource *r)  
> 
> Why inline? It could be in .c

OK.

> 
> > +{
> > +	return r->end - r->beg;
> > +}
> > +
> > +const struct resource *resource_find(const char *name);
> > +
> > +void __resource_register(struct resource *r);  
> 
> A comment is needed to explain the role of this function.
> Why a double underscore?

A non-API function. It shouldn't be called by user. Nothing more.

> 
> > +#define REGISTER_RESOURCE(_n, _b, _e) \  
> 
> I'm not a big fan of the underscores.

+1

> 
> > +static struct resource linkres_ ##_n = {       \
> > +	.name = RTE_STR(_n),     \
> > +	.beg = _b,               \
> > +	.end = _e,               \
> > +};                               \
> > +__REGISTER_RESOURCE(linkres_ ##_n)  
> 
> Please avoid nested macros.

I don't understand this. I am avoiding code duplication. Is it wrong?

> 
> > +#define __REGISTER_RESOURCE(name)     \
> > +static void __attribute__((constructor,used)) resinitfn_ ##name(void); \  
> 
> Why declaring the function just before its implementation?

Wrote in a hurry to work out-of-the box. Will fix...

> 
> > +static void __attribute__((constructor,used)) resinitfn_ ##name(void) \
> > +{                                   \
> > +	__resource_register(&name); \
> > +}  
> 



-- 
   Jan Viktorin                  E-mail: Viktorin at RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic