On Thu, Dec 11, 2014 at 12:03:37PM +0100, Max Reitz wrote: > On 2014-12-11 at 11:58, Stefan Hajnoczi wrote: > >On Wed, Dec 03, 2014 at 02:37:25PM +0100, Max Reitz wrote: > >>@@ -530,8 +530,16 @@ found: > >> } > >> /* XXX: cache several refcount block clusters ? */ > >>+/* In order to decrease refcounts, set @addend to the two's complement > >>(giving a > >>+ * negative value and letting the implicit cast handle it is enough) and > >>set > >>+ * @decrease to true. @decrease must be false if the refcount should be > >>+ * increased. */ > >The first time I read this patch I missed this quirk and thought that a > >lot of places seemed to be doing the wrong thing with addend. > > > >This is likely to cause confusion, why not make uint16_t addend truly > >unsigned and leave the sign to bool decrease, as suggested by the > >function prototype? > > Because it's very easy to call it with e.g. target_refcount - > current_refcount, and using an addition to apply the addend will always > work. > > So, the code is a bit shorter by doing this. On the other hand, I don't have > trouble making all callers do llabs(addend) or imaxabs(addend) (if the > absolute value is not known at compile time) and use addition or subtraction > in this function, depending on the boolean.
I prefer the solution using the absolute value because it makes the code idiot-proof for me to read :). Thanks, Stefan
pgp91ddVs0qK9.pgp
Description: PGP signature