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 :|
