svn commit: r332072 - head/sys/sys

[Roger Pau Monné]

On Fri, Apr 06, 2018 at 03:12:08AM +1000, Bruce Evans wrote:
> On Thu, 5 Apr 2018, Warner Losh wrote:
> 
> > On Thu, Apr 5, 2018 at 9:46 AM, Roger Pau Monné <royger at freebsd.org> wrote:
> > 
> > > On Thu, Apr 05, 2018 at 09:32:57AM -0600, Ian Lepore wrote:
> > > > On Thu, 2018-04-05 at 14:31 +0000, Roger Pau Monné wrote:
> > > > > Log:
> > > > >   introduce GiB and MiB macros
> > > > > ...
> > > > > +/* Unit conversion macros. */
> > > > > +#define GiB(v) (v ## ULL << 30)
> > > > > +#define MiB(v) (v ## ULL << 20)
> > > > > +
> > > > >  #endif     /* _SYS_PARAM_H_ */
> > > > 
> > > > These names don't make it clear whether the conversion is bytes->GiB or
> > > > GiB->bytes.  The names seem way too generic for a public namespace in a
> > > > file as heavily included behind your back as param.h is.
> > > > 
> > > > Also, this completely reasonable usage won't work, likely with
> > > > confusing compile error messages:
> > > > 
> > > >   int bytes, gibytes;
> > > >   ...
> > > >   bytes = GiB(gibytes);
> > > 
> > > I find those helpful for their specific usage. I could introduce
> > > static inline functions like:
> > > 
> > > size_t gb_to_bytes(size_t)...
> > > 
> > > But I assume this is also going to cause further discussion.
> 
> Yes, it gives even more namespace pollution and type errors.  Macros
> at least don't expose their internals if they are not used.
> 
> size_t is actually already part of the undocumented namespace pollution
> in <sys/param.h>.
> 
> The type errors are restriction to just one type in another way.  Type-
> generic APIs that avoid such restrictions are much harder to implement
> using inline functions than macros.
> 
> > Yea, traditional macro names would be "gibtob" and "btogib" but I didn't
> > just reply to bikeshed a name:
> > 
> > But you don't need to specify a type, consider the current btodb macro:
> > #define btodb(bytes)                    /* calculates (bytes / DEV_BSIZE)
> > */ \
> >        (sizeof (bytes) > sizeof(long) \
> >         ? (daddr_t)((unsigned long long)(bytes) >> DEV_BSHIFT) \
> >         : (daddr_t)((unsigned long)(bytes) >> DEV_BSHIFT))
> > 
> > which shows how to do this in a macro, which is orthogonal to any name you
> > may choose. I can also bikeshed function vs macro :)
> 
> This macro is mostly my mistake in 1995-1996.  The long long abominations
> in it were supposed to be temporary (until C99 standardized something
> better).  It was originally MD for i386 only and then the sizes of almost
> all types are known and fixed so it is easier to hard-code minimal sizes
> that work.  The optimization of avoiding using 64-bit types was more needed
> in 1995-1996 since CPUs were slower and compilers did less strength reduction.
> 
> btodb() is much easier than dbtob() since it shifts right, so it can't
> overflow unless the cast of 'bytes' is wrong so that it truncations.
> dbtob() doesn't try hard to be optimal.  It just always upcasts to
> off_t.
> 
> jake later convinced me (in connection with his PAE and sparc64 work) that
> it should be the caller's responsibility to avoid overflow.  Any casts in
> the macro limits it to the types in it.  This is why the page to byte
> conversion macros don't have any casts in them.  PAE usually needs 64-bit
> results, but this would just be a pessimization for normal i386, and
> deciding the casts in the macro as above is complicated.
> 
> So correct GB() macros would look like ((v) << 30), where the caller must
> cast v to a large enough type.  E.g., for variable v which might be larger
> than 4, on 32-bit systems, the caller must write something like
> GB((uintmax_t)v).  But it is easier for writing to just multiply v by 1G.
> This is also easier for reading since it is unclear that GB() is even a
> conversion or which direction it goes in.  A longer descriptive name would
> be about as clear and long as an explicit multiplication.
> 
> I usually write 1G as ((type)1024 * 1024 * 1024) since the decimal and
> even hex values of 1G have too many digits to be clear, and
> multiplication is clearer than shifting and allows the type to be in
> the factor.
> 
> Disk block size conversions need to use macros since the DEV_BSIZE = 512
> was variable in theory (in practice this is now a fixed virtual size).
> Conversions to G don't need macros since the magic number in them is no
> more magic than the G in their name.

I personally find the following chunk:

if (addr < GiB(4))
...

Much more easier to read and parse than:

if (addr < (4 * 1024 * 1024 * 1024))
...

But I won't insist anymore.

I will revert this and introduce the macros locally where I need them.

Roger.