Re: [PATCH 1/5] cfgfile: configurable comment character

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Friday, March 3, 2017 11:31 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolliffe@windriver.com>
> Subject: RE: [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> > Possible options that I see:
> > 1. Add a new parameters argument to the load functions (e.g. struct
> > cfgfile_params *p), whit the comment char as one (and currently only) field
> > of this struct. Drawbacks: API change that might have to be announced one
> > release before the actual API change.
> 
> I would prefer this option as it provides more flexibility.  We can leave the
> existing API as is and a wrapper that accepts additional parameters.
> Something like the following (with implementations in the .c obviously rather
> than inline the header like I have it here).  There are several examples of this
> pattern already in the dpdk (i.e., ring APIs, mempool APIs, etc.) where we
> use a common function invoked by higher level functions that pass in
> additional parameters to customize behavior.
> 
> struct rte_cfgfile *_rte_cfgfile_load(const char *filename,
> 				          const struct rte_cfgfile_params
> *params);
> 
> struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags)
> {
> 	struct rte_cfgfile_params params;
> 
> 	rte_cfgfile_set_default_params(&params);
> 	params |= flags;
> 	return _rte_cfgfile_load(filename, &params);
> }
> 
> struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
> 						    const struct
> rte_cfgfile_params *params)
> {
> 	return _rte_cfgfile_load(filename, params);
> }


Regardless of the approaches 1. or 2., we must control the acceptable set of chars for the comment separator, and the way to do this is through enum/flags.

Both approaches can support this. Therefore, IMO the separator char is not enough to justify approach 1. I would only go for approach 1 if there are some other parameters that we could consider adding to the load function now or later. Do you see any?