Re: G_TYPE_INT64

[Owen Taylor <otaylor redhat com>]

> Patch notes:
> 
> 1) To configure.in, i added:
> 
> #define G_MININT64     $glib_mll
> #define G_MAXINT64     $glib_Mll
> #define G_MAXUINT64    $glib_Mull
> 
> On my system, these are aliases to constants defined in
> /usr/include/limits.h:
> 
> #   define LLONG_MAX	9223372036854775807LL
> #   define LLONG_MIN	(-LLONG_MAX - 1LL)
> #   define ULLONG_MAX	18446744073709551615ULL
> 
> However, there appears to be some temporary confusion between glibc
> and gcc because these constants only work if some extra flag is passed
> to gcc, like '-std c99'.  Dunno, maybe this is correct behavior.

Well, the nice thint here is that G_MININT64 and so forth are the
same on all systems ;-), so I don't think we have to worry about
trying to figure out what system header these are in and defining
them in terms of that.

Simply:

#define G_MININT64  ((gint64) -0x8000000000000000)
#define G_MININT64  ((gint64)  0x7fffffffffffffff)
#define G_MAXUINT64 ((guint64) 0xffffffffffffffff)

Should work fine. (The casts shouldn't be necessary, strictly
speaking, if g[u]int64 is the smallest type that can hold
64 bit quantities, but make it a little more explicit.)

> 2) i ordered all the additions consistantly such that int64 comes
> between long and unichar.  The G_PARAM_SPEC_* defines were
> miss-ordered prior to my changes, so there are some additional edits
> there to fix things.

As mentioned earlier, I would have preferred to see this a 
separate patch, but assuming no changes got introduced that
I missed when moving stuff around, should be fine.
 
> 3) i removed G_HAVE_GINT64 conditionalization per Tim's suggestion.

<shrug> I think this doesn't make much sense, but am not going to
fight the issue.

_However_ just removing this from libgobject without removing it 
from configure and configure.in isn't the right thing to do. 

configure.in needs a check along the lines of:

 if [ $ac_cv_sizeof_long == 8 -o $ac_cv_sizeof_long_long == 8 ] ; then
   :
 else
   AC_MSG_ERROR([
*** GLib require a 64 bit type. You might want to consider
*** using the GNU C compiler.
])
 fi

G_HAVE_GINT64 should just be #define G_HAVE_GINT64 1, and all
references to G_HAVE_GINT64 conditionalization need to be remove.
To avoid a really huge patch, however, let's get this patch
committed and then worry about doing the rest.

> 4) i tried to follow the pattern in gvaluecollector.h, but i really
> have no idea if i did it correctly.  i don't understand this code and
> i'm not sure how to test it.  There is related code in gvaluetypes.c
> with which i have the similar reservations.  At least it compiles
> cleanly.  :-/

The way to test this would be to create a property of type GINT64
and trying to see if getting and setting it work correctly.

I'd really like to see a nice set of test programs for the various
components of libgobject.
 
 - Testing all GValue types
 - Testing all GParamSpec types
 - Testing signals

Etc. testgruntime is a start, but only tests a small subset of
GObject. I think a directory full small test programs would be a
better way of attacking this rather than one huge test program.

> @@ -1657,6 +1657,9 @@ G_BEGIN_DECLS
>  #define G_MINLONG	$glib_ml
>  #define G_MAXLONG	$glib_Ml
>  #define G_MAXULONG	$glib_Mul
> +#define G_MININT64	$glib_mll
> +#define G_MAXINT64	$glib_Mll
> +#define G_MAXUINT64	$glib_Mull
>  
>  _______EOF
>  
> @@ -1915,15 +1918,17 @@ esac
>  case xyes in
>  x$ac_cv_header_limits_h)
>    glib_limits_h=yes
> -  glib_ms=SHRT_MIN glib_Ms=SHRT_MAX glib_Mus=USHRT_MAX
> -  glib_mi=INT_MIN  glib_Mi=INT_MAX  glib_Mui=UINT_MAX
> -  glib_ml=LONG_MIN glib_Ml=LONG_MAX glib_Mul=ULONG_MAX
> +  glib_ms=SHRT_MIN    glib_Ms=SHRT_MAX    glib_Mus=USHRT_MAX
> +  glib_mi=INT_MIN     glib_Mi=INT_MAX     glib_Mui=UINT_MAX
> +  glib_ml=LONG_MIN    glib_Ml=LONG_MAX    glib_Mul=ULONG_MAX
> +  glib_mll=LLONG_MIN  glib_Mll=LLONG_MAX  glib_Mull=ULLONG_MAX
>    ;;
>  x$ac_cv_header_values_h)
>    glib_values_h=yes
> -  glib_ms=MINSHORT glib_Ms=MAXSHORT glib_Mus="(((gushort)G_MAXSHORT)*2+1)"
> -  glib_mi=MININT   glib_Mi=MAXINT   glib_Mui="(((guint)G_MAXINT)*2+1)"
> -  glib_ml=MINLONG  glib_Ml=MAXLONG  glib_Mul="(((gulong)G_MAXLONG)*2+1)"
> +  glib_ms=MINSHORT   glib_Ms=MAXSHORT   glib_Mus="(((gushort)G_MAXSHORT)*2+1)"
> +  glib_mi=MININT     glib_Mi=MAXINT     glib_Mui="(((guint)G_MAXINT)*2+1)"
> +  glib_ml=MINLONG    glib_Ml=MAXLONG    glib_Mul="(((gulong)G_MAXLONG)*2+1)"
> +  glib_mll=MINLLONG  glib_Mll=MAXLLONG  glib_Mull="(((guint64)G_MAXLLONG)*2+1)"

See above comment about what I think the right way to do this is.
If we weren't making that change, note the G_MAXLLONG and also,
I think it's pretty improbably that a system without the ANSI
standard <limits.h> would have a <values.h> that defined MAXLLONG.

>  static void
> +param_int64_init (GParamSpec *pspec)
> +{
> +  GParamSpecInt64 *lspec = G_PARAM_SPEC_INT64 (pspec);
> +  
> +#if SIZEOF_LONG_LONG == 8
> +  lspec->minimum = 0x7fffffffffffffff;
> +  lspec->maximum = 0x8000000000000000;
> +#else /* SIZEOF_LONG_LONG != 8 */
> +  lspec->minimum = 0x7fffffff;
> +  lspec->maximum = 0x80000000;
> +#endif
> +  lspec->default_value = 0;
> +}

This is pretty clearly wrong in various ways. I think

 lspec->minimum = G_MININT64;
 lspec->maximum = G_MAXINT64;

Would be the best approach ;-)

> +static void
> +param_uint64_init (GParamSpec *pspec)
> +{
> +  GParamSpecUInt64 *uspec = G_PARAM_SPEC_UINT64 (pspec);
> +  
> +  uspec->minimum = 0;
> +  uspec->maximum = ~0;

This gives the wrong result since ~0 is the same as ~(int)0
not ~(guint64)0. Again using G_MAXUINT64 would be the best
approach.

Other than that, looks fine. 

Why don't you go ahead and commit with the changes noted above
then I think we need in addition to that:

 - A patch to remove the rest of the conditionalization
   of G_HAVE_GINT64.

 - A patch adding test cases to make sure that G_TYPE_GUINT64 signals
   and values work correctly.

Thanks,
                                        Owen