Hi David, On Mon, Mar 14, 2016 at 07:54:24PM +0000, David CARLIER wrote: > Hi again, > > There is the same changes made in one patch to make the review more > effective as the changes, finally, depend to each other.
Great. I just spotted this, are you sure there isn't a mistake ? > -/* encoding of the arg count : up to 5 args are possible. 4 bits are left > +/* encoding of the arg count : up to 12 args are possible. 4 bits are left > * unused at the top. > */ > #define ARGM_MASK ((1 << ARGM_BITS) - 1) > -#define ARGM_BITS 3 > -#define ARGM_NBARGS (32 - ARGM_BITS) / sizeof(int) > +#define ARGM_BITS 4 > +#define ARGM_NBARGS (128- ARGM_BITS) / sizeof(int64_t) This 128 should very likely be 64 since we're moving to 64 bits. Also I'm having a hard time understanding why the number of bits is divided by the size of an int in either case. I guess it should have been something like this instead : (before) #define ARGM_BITS 3 #define ARGM_NBARGS (sizeof(int) * 8 - ARGM_BITS) / ARGT_BITS (after) #define ARGM_BITS 4 #define ARGM_NBARGS (sizeof(int64_t) * 8 - ARGM_BITS) / ARGT_BITS Indeed, we need to subtract the number of bits needed to count args from the whole int size, and divide it by the size of each individual arg to get the total number of args. The original version only happened to work by pure luck because sizeof(int) == ARGT_BITS! I think that deserves a fix in older versions eventhough it's a minor bug which cannot have any impact as-is. And in your case, that used to work as well because (128-4)/8 = 15.5 instead of 15.0, both of which are rounded down to 15. I remember that we always reserve one entry for ARGT_STOP, so the max usable number of args is 14 here. What do you think ? Just another detail here : - unsigned int arg_mask; /* arguments (ARG*()) */ + int64_t arg_mask; /* arguments (ARG*()) */ It's nicer to leave it unsigned (uint64_t), as seeing negative numbers for bitfields it really annoying when debugging. Thanks! willy
