* Dmitry Simonenko <[email protected]> [12/08/14 20:05]: > diff --git a/include/util.h b/include/util.h > index e438e95..7cbfa1d 100644 > --- a/include/util.h > +++ b/include/util.h > @@ -27,6 +27,7 @@ > */ > #include "config.h" > > +#include <string.h> > #include <unistd.h> > #include <inttypes.h>
What do you need this change for? > diff --git a/mod/box/box_lua.m b/mod/box/box_lua.m > index 654884f..abb136c 100644 > +static void > +lbox_pushtuple(struct lua_State *L, struct tuple *tuple); > + > +/** > + * Tuple transforming function. > + * > + * Removes the fields designated by 'offset' and 'len' from an tuple, > + * and replaces them with the elements of supplied data fields, > + * if any. > + * > + * Function returns newly allocated tuple. > + * It does not change any parent tuple data. > + * > + */ It's considered good style by some, including me, to use present tense, imperative mood in code comments, when describing what a function does. E.g. check this book out: http://shop.oreilly.com/product/0790145305770.do It's available online: http://books.google.ru/books?id=ifhpFdraIkQC&printsec=frontcover&hl=ru#v=onepage&q&f=false Extract: -- Do not use first person. For example, do not say 'Here we display a list of customers.' Use present tense for comments that describe a code block, and use imperative mood to describe the action within that code block. -- http://books.google.ru/books?id=ifhpFdraIkQC&printsec=frontcover&hl=ru#v=onepage&q&f=false Finally, the comment, apparently, is outdated. Here's how I think this comment should look like: /** * Given a tuple, range of fields to remove (start and end field * numbers), and a list of fields to paste, calculate the size of * the resulting tuple. * @param L lua stack, contains a list of arguments to paste * @param start offset in the lua stack where paste arguments * start * @param tuple old tuple * @param offset cut field offset * @param len how many fields to cut * @param[out] left size of the left part * @param[out] right size of the right part * @return size of the new tuple */ > +static size_t > +transform_calculate(struct lua_State *L, struct tuple *tuple, int start, > + int argc, > + int offset, int len, > + size_t *left, > + size_t *right) Style: arguments need to be aligned at the start of the opening bracket. We usually don't put each argument on its own line. > +{ > + /* calculating size of the new tuple */ Imperative mood, please. > + *left = tuple_sizeof(tuple, 0, offset); I believe tuple_sizeof should take a start pointer as the second argument, not a field number. This would save a bit of CPU on the second call. tuple_range_size, I think, would be a better name. It's perhaps worth making this function inline. > + /* calculating sizes of supplied fields */ > + size_t middle = 0; > + for (int i = start ; i <= argc ; i++) { > + size_t field_size = lua_objlen(L, i + 1); > + middle += varint32_sizeof(field_size) + field_size; > + } Why do you put i at 'start', and then use i+1 in the loop? This looks confusing, isn't it better to make sure all input parameters are in line so that the loop can run smoothly? > + /* calculating last part of the tuple fields */ > + *right = tuple_sizeof(tuple, offset + len, > + tuple->field_count - offset + len); > + return *left + middle + *right; > +} > + > +static int > +transform(struct lua_State *L, struct tuple *tuple, int start, > + int argc, > + int offset, int len) Style. > +{ > + /* validating offset and len */ > + if (offset < 0) { > + if (-offset > tuple->field_count) > + luaL_error(L, "tuple.transform(): offset is out of > bound"); > + offset += tuple->field_count; > + } else if (offset > tuple->field_count) { > + offset = tuple->field_count; > + } > + if (len < 0) > + luaL_error(L, "tuple.transform(): len is negative"); > + if (len > tuple->field_count - offset) > + len = tuple->field_count - offset; It seems you can't clearly separate input validation and the transform itself between lbox_tuple_transform() and transform(), so I would simply merge these two functions in one. > + /* calculating size of the new tuple */ > + size_t left = 0, right = 0; Why initialize variables if you are going to assign them later on? In fact, I would use an array. size_t sz[2]; -- less pointer arithmetics and less arguments to pass around. > + size_t size = transform_calculate(L, tuple, start, argc, offset, len, > + &left, > + &right); > + /* allocating new tuple */ > + struct tuple *dest = tuple_alloc(size); > + dest->field_count = (tuple->field_count - len) + > + (argc - (start - 1)); > + /* constructing tuple */ > + if (left) { > + memcpy(dest->data, tuple->data, left); > + } > + size_t off = left; > + for (int i = start ; i <= argc ; i++) { > + size_t field_size = 0; > + const char *field = luaL_checklstring(L, i + 1, &field_size); > + save_varint32(dest->data + off, field_size); > + off += varint32_sizeof(field_size); > + memcpy(dest->data + off, field, field_size); > + off += field_size; > + } If you use a pointer instead of offset for dst data in the loop, instead of size_t off, you have one less instruction per loop for the code to do (evaluating dest->data + off). Here you also start from i = start, yet use i + 1 inside the loop. I think it's less clear and less efficient. > + if (right) { > + memcpy(dest->data + off, > + tuple_field(tuple, offset + len), > + right); > + } memcpy already checks for zero copy length, an extra check for left or right is redundant. http://stackoverflow.com/questions/3751797/can-i-call-memcpy-and-memmove-with-number-of-bytes-set-to-zero > +static int > +tuple_find(struct lua_State *L, struct tuple *tuple, > + const char *key, > + size_t key_size, bool first) > +{ > + u8 *field = tuple->data; > + int fieldno = 0; > + int count = 0; > + while (fieldno < tuple->field_count) { > + size_t len = load_varint32((void **) &field); > + if (len == key_size && (memcmp(field, key, len) == 0)) { > + lua_pushinteger(L, fieldno); > + count++; > + if (first) > + break; > + } > + field += len; > + fieldno++; > + } > + return count; > +} You can rewrite this function to not use neither field_no nor count. Break the loop when field >= tuple->data + tuple->bsize. Save the stack top at the beginning, and return the diff. Otherwise the patch looks pretty nice, thank you for writing it! -- kostja _______________________________________________ Mailing list: https://launchpad.net/~tarantool-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~tarantool-developers More help : https://help.launchpad.net/ListHelp
