Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions

> Kousik Sanagavarapu <five231003@gmail.com> writes:
> 
> >  ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> 
> New helper functions that do not have any caller and no
> documentation to explain how they are supposed to be called
> (i.e. the expectation on the callers---what values they need to feed
> as parameters when they call these helpers, and the expectation by
> the callers---what they expect to get out of the helpers once they
> return) makes it impossible to evaluate if they are any good [*].
> 
> 	Side note.  Those of you who are keen to add unit tests to
> 	the system (Cc:ed) , do you think a patch line this one that
> 	adds a new helper function to the system, would benefit from
> 	being able to add a few unit tests for these otherwise
> 	unused helper functions?
> 
> 	The calls to the new functions that the unit test framework
> 	would make should serve as a good piece of interface
> 	documentation, showing what the callers are supposed to pass
> 	and what they expect, I guess.
> 
> 	So whatever framework we choose, it should allow adding a
> 	test or two to this patch easily, without being too
> 	intrusive.  Would that be a good and concrete evaluation
> 	criterion?
> 
> Anyway, because of that, I had to read [2/2] first and then come
> back here to review this one.
> 
> The following is my attempt to write down the contract between the
> callers and this new helper function---please give something like
> that to the final version.  The the example below is there just to
> illustrate the level of information that would be desired to help
> future readers and programmers.  Do not take the contents as-written
> as truth---I may have (deliberately) mixed in incorrect descriptions
> ;-).

> 
> /*
>  * The string "to_parse" is expected to be a comma-separated list
>  * of "key" or "key=val".  If your atom allows "key1" and "key2"
>  * (possibly with their values) as options, make two calls to this
>  * funtion, passing "key1" in candiate and then passing "key2" in
>  * candidate.
>  *
>  * The function Returns true ONLY when the to_parse string begins
>  * with the candidate key, possibly followed by its value (valueless
>  * key-only entries are allowed in the comman-separated list).
>  * Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and
>  * the function returns false.
>  *
>  * *valuestart will point at the byte after '=' (i.e. the beginning
>  * of the value), and the number of bytes in the value will be set
>  * to *valuelen.
>  * A key-only entry results in *valuestart set to NULL and *valuelen
>  * set to 0.
>  * *end will point at the next key[=val] in the comma-separated list
>  * or NULL when the list ran out.
>  */
> 
> > +static int match_atom_arg_value(const char *to_parse, const char *candidate,
> > +				const char **end, const char **valuestart,
> > +				size_t *valuelen)
> > +{
> > +	const char *atom;
> > +
> > +	if (!(skip_prefix(to_parse, candidate, &atom)))
> > +		return 0;
> > +	if (valuestart) {
> 
> As far as I saw, no callers pass NULL to valuestart.  Getting rid of
> this if() statement and always entering its body would clarify what
> is going on, I think.
> 
> > +		if (*atom == '=') {
> > +			*valuestart = atom + 1;
> > +			*valuelen = strcspn(*valuestart, ",\0");
> > +			atom = *valuestart + *valuelen;
> > +		} else {
> > +			if (*atom != ',' && *atom != '\0')
> > +				return 0;
> > +			*valuestart = NULL;
> > +			*valuelen = 0;
> > +		}
> > +	}
> > +	if (*atom == ',') {
> > +		*end = atom + 1;
> > +		return 1;
> > +	}
> > +	if (*atom == '\0') {
> > +		*end = atom;
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> 
> /*
>  * Write something similar to document the contract between the caller
>  * and this function here.
>  */
> > +static int match_atom_bool_arg(const char *to_parse, const char *candidate,
> > +				const char **end, int *val)
> > +{