[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "q
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h" |
Date: |
Fri, 29 Jun 2018 16:02:36 +0100 |
User-agent: |
Mutt/1.10.0 (2018-05-17) |
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 :|
- [Qemu-devel] [PATCH v5 00/46] Use the IEC binary prefix definitions, Philippe Mathieu-Daudé, 2018/06/25
- [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Richard Henderson, 2018/06/27
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Igor Mammedov, 2018/06/27
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Eric Blake, 2018/06/27
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/28
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Eric Blake, 2018/06/29
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/29
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h",
Daniel P . Berrangé <=
- Re: [Qemu-devel] [PATCH v5 01/46] include: Add IEC binary prefixes in "qemu/units.h", Eric Blake, 2018/06/29
[Qemu-devel] [PATCH v5 02/46] vdi: Use definitions from "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25
[Qemu-devel] [PATCH v5 03/46] x86/cpu: Use definitions from "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25
[Qemu-devel] [PATCH v5 04/46] checkpatch: Recognize IEC binary prefix definitions, Philippe Mathieu-Daudé, 2018/06/25
[Qemu-devel] [PATCH v5 05/46] hw: Use IEC binary prefix definitions from "qemu/units.h", Philippe Mathieu-Daudé, 2018/06/25