branches/binary-1.3.0

[Tomash Brechko]

Hello Dustin,

A small comment on your binary protocol patch.


> +static void process_bin_get(conn *c) {
> 
> ...
> 
> +        /* if it's a gets, add the identifier */
> +        if(c->cmd == CMD_GETS) {
> +            uint64_t* identifier;
> +            identifier=(uint64_t*)(c->wbuf + MIN_BIN_PKT_LENGTH + 4);
> +            *identifier=swap64((uint32_t)it->cas_id);

Casting CAS to uint32_t is probably from the times when it was done so
in the mainline, you don't need it any more.

BTW, all this (uint64_t*), (int*) casts call for a BIG FAT WARNING
that we heavily depend on the fact that the address is properly
aligned for this type, i.e. dereferencing uint64_t* not aligned to 8
bytes would trap on some platforms.


> +#define MIN_BIN_PKT_LENGTH 12
> 
> ...
> 
> +#define BIN_PKT_HDR_WORDS (MIN_BIN_PKT_LENGTH/sizeof(uint32_t))

Not really important, but by putting this the other way around you
would show that one thing is the exact multiple of the other:

   #define BIN_PKT_HDR_WORDS 3
   #define MIN_BIN_PKT_LENGTH (BIN_PKT_HDR_WORDS * sizeof(uint32_t))


> +    /* Binary protocol stuff */
> +    /* This is where the binary header goes */
> +    uint32_t bin_header[MIN_BIN_PKT_LENGTH/sizeof(uint32_t)];

Better

       uint32_t bin_header[BIN_PKT_HDR_WORDS];


-- 
   Tomash Brechko