[ccan] [PATCH 1/3] mem: add mem helper functions

[David Gibson]

On Tue, Aug 18, 2015 at 10:54:52PM -0400, Cody P Schafer wrote:
> On Tue, Aug 18, 2015 at 7:08 PM, David Gibson
> <david at gibson.dropbear.id.au> wrote:
> > On Mon, Aug 17, 2015 at 08:33:29PM -0400, Cody P Schafer wrote:
> >> Signed-off-by: Cody P Schafer <dev at codyps.com>
> >
> > Concept looks fine.  A number of minor nits.
> >
> >> ---
> >> +/**
> >> + * memstarts - determine if @data starts with @prefix
> >> + * @data: does this begin with @prefix?
> >> + * @data_len: bytes in @data
> >> + * @prefix: does @data begin with these bytes?
> >> + * @prefix_len: bytes in @prefix
> >> + *
> >> + * Returns true if @data starts with @prefix, otherwise return false.
> >> + *
> >> + * Example:
> >> + *   if (memstarts(somebytes, bytes_len, otherbytes, otherbytes_len)) {
> >> + *           printf("somebytes starts with otherbytes!\n");
> >> + *   }
> >> + */
> >> +static inline bool memstarts(void const *data, size_t data_len,
> >> +             void const *prefix, size_t prefix_len)
> >> +{
> >> +     if (prefix_len > data_len)
> >> +             return false;
> >> +     return !memcmp(data, prefix, prefix_len);
> >
> > I'd prefer to see the order changed, and use memeq() here.
> 
> I don't think memeq() fits here, as it includes the length comparison,
> which is required when checking if 2 blocks of memory are equal, but
> serves no purpose here.
> 
> I'd need to either:
>  - pass prefix_len twice (ugly)

I think this is correct, and I don't see it as that ugly.

>  - remove the memstarts length check and use MIN(prefix_len,
> data_len), which makes it harder to think about the function, and it
> seems silly to generate a value we know will cause the function to
> return false rather than just returning false directly.
> 
> Neither seems really great.
> 
> >
> >> +}
> >> +
> >> +/**
> >> + * memeq - Are two byte arrays equal?
> >> + * @a: first array
> >> + * @al: bytes in first array
> >> + * @b: second array
> >> + * @bl: bytes in second array
> >> + *
> >> + * Example:
> >> + *   if (memeq(somebytes, bytes_len, otherbytes, otherbytes_len)) {
> >> + *           printf("memory blocks are the same!\n");
> >> + *   }
> >> + */
> >> +#define memeq(a, al, b, bl) (al == bl && !memcmp(a, b, bl))
> >
> > Needs brackeds around the macro arguments, in case they're complex
> > expressions.
> 
> Are you looking for just the following? :
> 
> +#define memeq(a, al, b, bl) ((al) == (bl) && !memcmp(a, b, bl))
> 
> Or should the function args be paren'd as well?

The function args should have parens as well - consider what happens
if one of the params is a comma expression (of course it's not trivial
to get an un-parenthised comma expression into a macro argument, but
you should still handle it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20150819/4b82cca8/attachment-0001.sig>