After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 561812 - Support time_t
Support time_t
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2008-11-21 14:57 UTC by Colin Walters
Modified: 2008-12-01 18:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Support time_t (3.31 KB, patch)
2008-11-21 14:57 UTC, Colin Walters
none Details | Review
Support time_t (4.29 KB, patch)
2008-11-21 17:22 UTC, Colin Walters
needs-work Details | Review
Support time_t (4.69 KB, patch)
2008-11-21 19:23 UTC, Colin Walters
none Details | Review
Support time_t (4.23 KB, patch)
2008-11-21 20:16 UTC, Colin Walters
none Details | Review

Description Colin Walters 2008-11-21 14:57:21 UTC
This patch assumes that the host system uses "long" for time_t, which
is true for GNU systems.  Right now gobject-introspection also assumes
this.
Comment 1 Colin Walters 2008-11-21 14:57:30 UTC
Created attachment 123175 [details] [review]
Support time_t
Comment 2 Tommi Komulainen 2008-11-21 15:18:04 UTC
Comment on attachment 123175 [details] [review]
Support time_t

>@@ -333,6 +333,8 @@ gjs_value_to_g_argument(JSContext      *context,
> #if (GLIB_SIZEOF_LONG == 8)
>     case GI_TYPE_TAG_LONG:
>     case GI_TYPE_TAG_SSIZE:
>+    /* TODO - support systems where time_t is a double, etc. */
>+    case GI_TYPE_TAG_TIME_T:
> #endif

Isn't this missing GLIB_SIZEOF_LONG == 4 case?
Can't see if it would support Date -> time_t conversion either, not entirely sure it should even.


>+jsval
>+gjs_date_from_long (JSContext *context, long time)
>+{
>+    return js_NewDateObjectMsec (context, (double) time);
>+}

Hmm, this looks like private JS API. I don't think we're supposed to call it.


>+    let now = new Date();
>+    let retDate = Everything.test_timet(now);
>+    /* Just test year/month since JS represents times as
>+     * doubles internally (whereas GNU libc uses long), and
>+     * I am skeptical about this test not randomly failing
>+     * later if we try to be completely precise.  This
>+     * conversion though is likely to be good enough for most
>+     * purposes.
>+     */
>+    assertEquals(now.getFullYear(), retDate.getFullYear());
>+    assertEquals(now.getMonth(), retDate.getMonth());

I think this should test up to the second. At least that's pretty much the expected precision usually for time_t.
Comment 3 Owen Taylor 2008-11-21 15:25:46 UTC
+jsval
+gjs_date_from_long (JSContext *context, long time)
+{
+    return js_NewDateObjectMsec (context, (double) time);
+}

I don't understand how that works ... as far as I can see 
js_NewDateObjectMsec() wants a Java style milliseconds since the epoch,
not a Unix style seconds-since epoch. (And as Tommi said, js_* stuff
is generally private API, AFAIK only the JS_* stuff in jsapi.h is
supposed to be public.)

IMO you do want bidirectional conversion to Date. What's unclear to
me is if passing a number in instead should be supported. If you do
support it, is it a seconds or a milliseconds value?

Is a timet something like a whippet?
Comment 4 Tommi Komulainen 2008-11-21 15:35:45 UTC
(In reply to comment #3) 
> IMO you do want bidirectional conversion to Date. What's unclear to
> me is if passing a number in instead should be supported. If you do
> support it, is it a seconds or a milliseconds value?

I'd think we'd use doubles for seconds. time_t as long doesn't support higher precision anyway, and when it's represented as double there'd be direct mapping anyway.
Comment 5 Owen Taylor 2008-11-21 16:08:52 UTC
(In reply to comment #4)

> I'd think we'd use doubles for seconds. time_t as long doesn't support higher
> precision anyway, and when it's represented as double there'd be direct mapping
> anyway.
 
That is almost certainly right; the only reason I was hesitating at all
was the plan to make

 widget.set_rectangle({ x: 2, y: 2, width: 10, height: 10 });

The same as:

 widget.set_rectangle(new Gdk.Rectangle({ x: 2, y: 2, width: 10, height: 10 }));

Then it's a little weird to me if:

 widget.set_time(342123412);

works but isn't the same as:

 widget.set_time(new Date(342123412));
Comment 6 Tommi Komulainen 2008-11-21 16:34:52 UTC
Right, it is a little weird, though the cases are subtly different.

set_rectangle() expects a Rectangle which we can implicitly construct from {..} -- or you can explicitly create a Rectangle yourself

set_time() expects time_t which we can implicitly construct from 342123412 (numbers) or Date objects -- if you explicitly create Date objects we expect you know how to do that properly

I guess what I'm trying to say is that set_time() expects time_t, not Date, which makes the difference. (If it expected Date the inequality between the two would be a big mistake IMO.)
Comment 7 Colin Walters 2008-11-21 17:22:32 UTC
Created attachment 123185 [details] [review]
Support time_t

This patch assumes that the host system uses "long" for time_t, which
is true for GNU systems.  Right now gobject-introspection also assumes
this.
Comment 8 Colin Walters 2008-11-21 17:24:44 UTC
The above patch does not work - I can't figure out how to get a reference to the JS Date "class".  All the code I can find around on the web is mozilla internals which are using the internal API, and the public API points around this stuff are undocumented and obscure.

It does address some of the above comments though.
Comment 9 Havoc Pennington 2008-11-21 17:30:29 UTC
A hacky way to get the JSClass* for Date would be to eval "new Date()" and then call JS_GetClass() on the result. But surely there is some more sane alternative to that ;-)
Comment 10 Tommi Komulainen 2008-11-21 17:34:34 UTC
Date is a global JS class / function, so you should be able to just call it with the right arguments. See 
https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Phrasebook#Calling_a_global_JS_function

and 

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Phrasebook#throw -- look for JS_CallFunctionName
Comment 11 Owen Taylor 2008-11-21 18:07:29 UTC
I believe if you call "Date()" you'll get a string back, as if you did
it in JS. There doesn't seem to be any way to invoke a function as a
constructor in the jsapi.h other than JS_ConstructObject.

A guess for getting the class

 Use JS_GetProperty to get the "prototype" property of the Date function
 Call JS_GetClass on that
Comment 12 Colin Walters 2008-11-21 19:23:39 UTC
Created attachment 123189 [details] [review]
Support time_t

This patch assumes that the host system uses "long" for time_t, which
is true for GNU systems.  Right now gobject-introspection also assumes
this.
Comment 13 Owen Taylor 2008-11-21 19:33:34 UTC
+#if (GLIB_SIZEOF_LONG == 8)
+                                        (time_t) arg->v_long);
+#else
+                                        (time_t) arg->v_int);
+#endif

If GLIB_SIZEOF_LONG == 4, isn't arg->v_long the same as arg->v_int?

If not, this will break with gfield.c - since that does:

  value->v_long = G_STRUCT_MEMBER(time_t, mem, offset);

To read a time_t field.


+        /* FIXME - using floating point for this is broken */
+        if (!JS_ValueToNumber(context, value, &v))
+            wrong = TRUE;

Is not :-)

+    if (!localtime_r (&time, &broken))
+        gjs_fatal("Failed to convert time");
+
+    args[0] = INT_TO_JSVAL(1900 + broken.tm_year);
+    args[1] = INT_TO_JSVAL(broken.tm_mon);
+    args[2] = INT_TO_JSVAL(broken.tm_mday);
+    args[3] = INT_TO_JSVAL(broken.tm_hour);
+    args[4] = INT_TO_JSVAL(broken.tm_min);
+    args[5] = INT_TO_JSVAL(broken.tm_sec);
+    args[6] = INT_TO_JSVAL(0);

OTOH, that...

Looking at jsdate.c:
JS_FRIEND_API(JSObject *)
js_NewDateObject(JSContext* cx, int year, int mon, int mday,
                 int hour, int min, int sec)
{
    JSObject *obj;
    jsdouble msec_time;

    JS_ASSERT(mon < 12);
    msec_time = date_msecFromDate(year, mon, mday, hour, min, sec, 0);
    obj = js_NewDateObjectMsec(cx, UTC(msec_time));
    return obj;
}

So instead of just converting to a double and multiplying by 1000, 
you looked up timezone data, and did two complex calender computations
(involving leap seconds and who knows what else...) to convert it
to a different representation and back again.

Comment 14 Colin Walters 2008-11-21 20:16:24 UTC
Created attachment 123193 [details] [review]
Support time_t

This patch assumes that the host system uses "long" for time_t, which
is true for GNU systems.  Right now gobject-introspection also assumes
this.
Comment 15 Colin Walters 2008-11-21 20:17:46 UTC
Yeah, I guess it makes sense to just give up and code to SpiderMonkey since that's what gjs is.  Fixed the first sizeof issue, thanks.
Comment 16 Johan (not receiving bugmail) Dahlin 2008-11-27 23:43:12 UTC
(In reply to comment #14)
> Created an attachment (id=123193) [edit]
> Support time_t
> 
> This patch assumes that the host system uses "long" for time_t, which
> is true for GNU systems.  Right now gobject-introspection also assumes
> this.
> 

This looks good to me. Did you leak check it? (make valgrind-check iirc)
Comment 17 Colin Walters 2008-12-01 18:20:59 UTC
Sorry for the delay - to verify I hadn't introduced any new leaks I had to do some valgrind cleanup (bug 562892).

This patch is now committed.

Committed r123
	M	test/js/testEverythingBasic.js
	M	gjs/jsapi-util.c
	M	gjs/jsapi-util.h
	M	gi/arg.c
r123 = 6cdf0495eb4980c3e471b65a6bc2b6c148ba34bf (git-svn)