Re: [Qemu-trivial] [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h"

[Daniel P . Berrangé]

On Fri, Jun 29, 2018 at 11:49:51AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Eric,
> 
> On 06/29/2018 09:19 AM, Eric Blake wrote:
> > On 06/28/2018 05:53 PM, Philippe Mathieu-Daudé wrote:
> > 
> >>>>> +#ifndef QEMU_UNITS_H
> >>>>> +#define QEMU_UNITS_H
> >>>>> +
> >>>>> +#define KiB     (INT64_C(1) << 10)
> >>>>> +#define MiB     (INT64_C(1) << 20)
> >>>>> +#define GiB     (INT64_C(1) << 30)
> >>>>> +#define TiB     (INT64_C(1) << 40)
> >>>>> +#define PiB     (INT64_C(1) << 50)
> >>>>> +#define EiB     (INT64_C(1) << 60)
> >>>> Shouldn't above use UINT64_C()
> >>>
> >>> Since the decision of signed vs. unsigned was intentional based on
> >>> review on earlier versions, it may be worth a comment in this file that
> >>> these constants are intentionally signed (in usage patterns, these tend
> >>> to be multiplied by another value; and while it is easy to go to
> >>> unsigned by doing '1U * KiB', you can't go in the opposite direction if
> >>> you want a signed number for '1 * KiB' unless KiB is signed).
> >>
> >> OK.
> > 
> > Actually, '1U * KiB' still ends up signed.  Why? Because as written, KiB
> > is a 64-bit quantity, but 1U is 32-bit; type promotion says that since a
> > 64-bit int can represent all 32-bit unsigned values, the result of the
> > expression is still signed 64-bit.
> 
> Are you suggesting this?
> 
> #define KiB     (INT32_C(1) << 10)
> #define MiB     (INT32_C(1) << 20)
> #define GiB     (INT32_C(1) << 30)
> 
> #define TiB     (INT64_C(1) << 40)
> #define PiB     (INT64_C(1) << 50)
> #define EiB     (INT64_C(1) << 60)

This feels dangerous to me as now the calculation may or may not be 32-bit
or 64-bit depending on which constant you happen to pick. I think this
inconsistency is going to be surprising to both devs and reviewers leading
to bugs.

Did you consider just adding unsigned versions  ?  eg UKiB, UMiB, UGiB. It
is a bit ugly but is has the benefit of being obvious whether you're intending
the calculation  to be signed vs unsigned.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|