On Sat, Sep 12, 2015 at 08:45:43PM -0400, Michael McConville wrote:
> Nicholas Marriott wrote:
> > Works for me. ok anyone?
> >
> > I think ksh_limval.h can go entirely after this, per the note in
> > PROJECTS.
>
> I also just found this gem. It only has one use, so it can probably be
> replaced. Am I interpreting it correctly?
No this is not right. long can still be 32 or 64 bits on different
platforms (such as i386 (ILP32) and amd64 (I32LP64)).
You need to keep the flags checks because long long or a 64-bit long
take up more space on the stack than an int or a 32-bit long - if you
ask va_arg for a long long when there is only an int or a 32-bit long,
it will use the wrong size.
The second check is kind of confusing, because not only was the comment
clearly not updated when changing from short/int to int/long but the
macro itself was not updated when llnum was changed from unsigned long
to unsigned long long.
When llnum was unsigned long, the intent was that -ve numbers were sign
extended for %d but not %[oux]. So where int is 32 bits and long 64
bits, -1 to %d gave (unsigned long)(int)-1 but -1 to %x gave (unsigned
long)(unsigned int)-1. Where int and long are both 32 bits, it didn't
matter.
But llnum is now always 64 bits (unsigned long long) even when long is
32 bits, so the check is now wrong and will not sign extend when it
should (for example on i386).
So the (sizeof (int) < sizeof (long) condition can be removed - now that
llnum is long long, it should actually be sizeof (int) < sizeof (long
long), which is always true on OpenBSD. But we need to keep the true
branch not the false, so that the sign extension happens correctly for
[di] vs [oux].
There is another problem: %d is checked, but not %i - I think they
should be the same.
Also if it is concerned about sign extension for %d vs %x, then it
should be for %ld vs %lx too, because %ld and %d are the same on ILP32
platforms.
So the macro should possibly be expanded to something like:
if (flags & FL_LLONG)
llnum = va_arg(args, unsigned long long);
else if (flags & FL_LONG) {
if (c == 'd' || c == 'i')
llnum = va_arg(args, long);
else
llnum = va_arg(args, unsigned long);
} else {
if (c == 'd' || c == 'i')
llnum = va_arg(args, int);
else
llnum = va_arg(args, unsigned int);
}
I hope this makes sense...
>
>
> Index: shf.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/shf.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 shf.c
> --- shf.c 19 Apr 2013 17:36:09 -0000 1.16
> +++ shf.c 13 Sep 2015 00:42:51 -0000
> @@ -715,21 +715,6 @@ shf_smprintf(const char *fmt, ...)
> * constant, just use a large buffer.
> */
>
> -/*
> - * What kinda of machine we on? Hopefully the C compiler will optimize
> - * this out...
> - *
> - * For shorts, we want sign extend for %d but not for %[oxu] - on 16 bit
> - * machines it don't matter. Assumes C compiler has converted shorts to
> - * ints before pushing them.
> - */
> -#define POP_INT(f, s, a) \
> - (((f) & FL_LLONG) ? va_arg((a), unsigned long long) : \
> - ((f) & FL_LONG) ? va_arg((a), unsigned long) : \
> - (sizeof(int) < sizeof(long) ? ((s) ? \
> - (long) va_arg((a), int) : va_arg((a), unsigned)) : \
> - va_arg((a), unsigned)))
> -
> #define ABIGNUM 32000 /* big numer that will fit in a short */
> #define LOG2_10 3.321928094887362347870319429 /* log base 2
> of 10 */
>
> @@ -890,7 +875,7 @@ shf_vfprintf(struct shf *shf, const char
> case 'x':
> flags |= FL_NUMBER;
> s = &numbuf[sizeof(numbuf)];
> - llnum = POP_INT(flags, c == 'd', args);
> + llnum = va_arg(args, unsigned long long);
> switch (c) {
> case 'd':
> case 'i':
