On Tue, Jul 26, 2016 at 02:35:53PM +0300, Uri Lublin wrote: > On 07/25/2016 07:51 PM, Francois Gouget wrote: > > On Mon, 25 Jul 2016, Frediano Ziglio wrote: > > [...] > > > > > - "client-tokens-interval", REDS_TOKENS_TO_SEND, > > > > > - "self-tokens", REDS_NUM_INTERNAL_AGENT_MESSAGES, > > > > > + "client-tokens-interval", (guint64)REDS_TOKENS_TO_SEND, > > > > > + "self-tokens", (guint64)REDS_NUM_INTERNAL_AGENT_MESSAGES, > > [...] > > > > The patch is ok, but I think it would be better > > > > to do the cast in the define itself, or replace > > > > the define with a const (g)uint64_t variable > > > > > > > > Uri. > > > > > > This would bound the constant to the property which does > > > not make much sense as the constant can be used for > > > different purposes. What if the same constant is used for > > > two properties with different types? > > I do not see it as a problem. Constants have meaning. > What if the constant is used in 10 places (as uint64), you > will need to cast all places. > (Theoretically, not in this case as Francois mentions below) > > > > > To be fair REDS_TOKENS_TO_SEND is only used in this one place. But > > REDS_NUM_INTERNAL_AGENT_MESSAGES is defined in main-channel.h and is > > used to define MAIN_CHANNEL_RECEIVE_BUF_SIZE so defining it as a guint64 > > may not make sense. > > > > Also I think having a visible cast here more explicitly indicates that > > the property is 64 bit than a cast hidden in a far away macro. > > > > (One could also argue for an explicit comment but I think that would be > > overkill. Why add a comment here and not for every other cast?) > > > > I do not see the value of being explicit about this property being > a 64 bit. I agree that a comment would be an overkill. > > Having said that, I accept the patch as is. Ok, I've pushed this patch then, thanks everyone! Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel