Messages in this thread | | | Subject | Re: [PATCH v1 1/8] lib/string: introduce match_string() helper | From | Andy Shevchenko <> | Date | Fri, 08 Jan 2016 10:40:51 +0200 |
| |
On Thu, 2016-01-07 at 23:05 +0100, Rasmus Villemoes wrote: > On Thu, Jan 07 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.c > om> wrote: > > > From time to time we have to match a string in an array. Make a > > simple helper > > for that purpose.
> > /** > > + * match_string - matches given string in an array > > + * @array: array of strings > > + * @len: number of strings in the array or 0 for NULL > > terminated arrays > > + * @string: string to match with > > + * > > + * Return: > > + * index of a @string in the @array if matches, or %-ENODATA > > otherwise. > > + */ > > +int match_string(const char * const *array, size_t len, const char > > *string) > > +{ > > + int index = 0; > > + const char *item; > > + > > + do { > > + item = array[index]; > > + if (!item) > > + break; > > + if (!strcmp(item, string)) > > + return index; > > + } while (++index < len || !len); > > + > > + return -ENODATA; > > +} > > +EXPORT_SYMBOL(match_string); > > + > > I'd suggest making it -1 (which, since len is a size_t, is > effectively > infinity) having the meaning "the array is terminated by a NULL > entry". match_string(..., ARRAY_SIZE(my_array), ...) will break if > the > array happens to be empty, which could e.g. happen in a case like > > const char *my_array[] = { > #ifdef CONFIG_THIS > "this", > #endif > #ifdef CONFIG_THAT > "that", > #endif > };
It might make sense, though I don't remember current users with such conditions.
> I also think the condition/loop above is unreadable.
Hmm… For me looks straightforward.
> > for (index = 0; index < len; index++) { > ... > } > > is much clearer.
If we switch to -1, it will look indeed simpler.
> > Why -ENODATA and not just -1? It is rather unlikely that anyone would > pass on that particular -Exxx value. Not a biggie, just curious.
There are few of users already that would like to return error code to upper level. In some cases better to have
return match_string();
than
ret = match_string(); if (ret < 0) return -EFOO;
return 0;
And returning -ENODATA doesn't prevent to have latter, but allows former.
> > Would there be more potential users if we had a flag argument > allowing > case-insensitive matching? Would there be more potential users if a > flag > allowed to ask whether the given string is a _prefix_ of one of the > strings in the array, or vice versa? Something like > > #define MATCH_STRING_CASE 0x01 > #define MATCH_STRING_PREFIX_OF_ARRAY_ELEM 0x02 /* yeah, that name > sucks */ > #define MATCH_ARRAY_ELEM_PREFIX_OF_STRING 0x04 /* this too */ > > int match_string(const char * const *array, size_t len, const char > *string, unsigned flags) > { > #define MATCH_PREFIX (MATCH_... | MATCH_...) > int index; > const char *item; > int (*match_func)(const char *, const char *) = > flags & MATCH_STRING_CASE ? strcasecmp : strcmp; > int (*prefix_func)(const char *, const char *, size_t) = > flags & MATCH_STRING_CASE ? strncasecmp : strncmp; > > for (index = 0; index < len; ++index) { > item = array[index]; > if (!item) > break; > if (flags & MATCH_PREFIX) { > size_t len = strlen(flags & > MATCH_STRING_PREFIX_OF_ARRAY_ELEM ? > string : item); > if (!prefix_func(item, string, len)) > return index; > } else if (!match_func(item, string)) { > return index; > } > } > return -1; > } > > (Ok, it's not that pretty; maybe it'd be better to use > switch(flags&MATCH_PREFIX) {}. Or maybe just the case-insensitive > part > is worth keeping; in that case the above isn't that bad.)
I won't overcomplicate it until we have enough users to consider. Any examples where we need this?
And I prefer way to have different prototypes for them instead of net of conditions.
Thanks for review. I will send v3 (yeah, this is actually v2) with change you proposed in the first part. For the second one I would like to have real examples before doing anything.
-- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
| |