Bug 34983 - Internally use GDateTime for date computation
Summary: Internally use GDateTime for date computation
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: logger (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ni...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-03-03 12:51 UTC by Nicolas Dufresne
Modified: 2011-03-16 18:46 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Nicolas Dufresne 2011-03-03 12:51:25 UTC
Currently logger uses a small helper library called datetime to do date computation. Instead, it should use GDateTime as it always uses gint64 representation instead time_t, which size depends on CPU.
Comment 2 Danielle Madeley 2011-03-03 17:01:09 UTC
This looks pretty good.

I had originally wondered if you should just pass GDateTimes around in the API, but then you have to manage their memory which seems like a pain in the neck.

The only potential downfall I can see of using gint64s throughout is that you could potentially mix up localtime and unix time? You code here looks correct, but it's a mistake someone could possibly make in the future? This could potentially be mitigated with a TplUnixTimestamp64 typedef, although it then requires people to use it and think and understand, so maybe it's not useful.

Does this branch remove all of the time_ts from the API? I thought you'd said there were still some left in the public API, but greping didn't find them.
Comment 3 Nicolas Dufresne 2011-03-03 20:17:30 UTC
(In reply to comment #2)
> This looks pretty good.
> 
> I had originally wondered if you should just pass GDateTimes around in the API,
> but then you have to manage their memory which seems like a pain in the neck.

Actually, one of my goal is to replace the gint64 timestamp in TplEvent with a GDateTime. As GDateTime is immutable and ref-counted, I never have to copy it. As most operation simply require generating formated strings I don't even have to reference it. But this requires breaking API, so I decided to do it in two steps, this is first and second is to actually break the API and integrate it in future 0.3.X branch.

> 
> The only potential downfall I can see of using gint64s throughout is that you
> could potentially mix up localtime and unix time? You code here looks correct,
> but it's a mistake someone could possibly make in the future? This could
> potentially be mitigated with a TplUnixTimestamp64 typedef, although it then
> requires people to use it and think and understand, so maybe it's not useful.

It's indeed a risk, GDateTime is even a better solution since it abstract the timezone. Creating TplUnixTimestamp64 does not seem bad, and does not actually break the API, I'll consider this.

> 
> Does this branch remove all of the time_ts from the API? I thought you'd said
> there were still some left in the public API, but greping didn't find them.

There was no time_t in the public API, but this change removes all use of time_t in the internal implementation. For 0.3.X I also plan on replacing GDate with GDateTime in list return by _get_dates() and inside TplSearchHit.
Comment 4 Danielle Madeley 2011-03-03 21:12:32 UTC
(In reply to comment #3)

> Actually, one of my goal is to replace the gint64 timestamp in TplEvent with a
> GDateTime. As GDateTime is immutable and ref-counted, I never have to copy it.
> As most operation simply require generating formated strings I don't even have
> to reference it. But this requires breaking API, so I decided to do it in two
> steps, this is first and second is to actually break the API and integrate it
> in future 0.3.X branch.

Ahh, so it will return a (transfer none) pointer to the GDateTime that's owned by the object?

> It's indeed a risk, GDateTime is even a better solution since it abstract the
> timezone.

That was my thinking.

> There was no time_t in the public API, but this change removes all use of
> time_t in the internal implementation. For 0.3.X I also plan on replacing GDate
> with GDateTime in list return by _get_dates() and inside TplSearchHit.

Is there a problem with using GDate, it hasn't been deprecated afaik, and doesn't have wrapping problems, since it's just a struct. It makes sense for get_dates() because it's just a date (no time). It is mutable, but we only ever pass copies as transfer-full right?, so that's not a big deal. If you did use GDateTimes what timezone would you use in them?
Comment 5 Nicolas Dufresne 2011-03-04 05:57:11 UTC
(In reply to comment #4)
> (In reply to comment #3)
> 
> > Actually, one of my goal is to replace the gint64 timestamp in TplEvent with a
> > GDateTime. As GDateTime is immutable and ref-counted, I never have to copy it.
> > As most operation simply require generating formated strings I don't even have
> > to reference it. But this requires breaking API, so I decided to do it in two
> > steps, this is first and second is to actually break the API and integrate it
> > in future 0.3.X branch.
> 
> Ahh, so it will return a (transfer none) pointer to the GDateTime that's owned
> by the object?
Yes.

> > There was no time_t in the public API, but this change removes all use of
> > time_t in the internal implementation. For 0.3.X I also plan on replacing GDate
> > with GDateTime in list return by _get_dates() and inside TplSearchHit.
> 
> Is there a problem with using GDate, it hasn't been deprecated afaik, and
> doesn't have wrapping problems, since it's just a struct. It makes sense for
> get_dates() because it's just a date (no time). It is mutable, but we only ever
> pass copies as transfer-full right?, so that's not a big deal. If you did use
> GDateTimes what timezone would you use in them?

Actually your right, only the time part of GDate has been deprecated. GDate can
be kept. Also, it allow not to care about timezone and risk that the represented 
date changes because of current locale settings. The goal of those dates are to
present days as the user remember, so regardless of the timezone.
Comment 6 Nicolas Dufresne 2011-03-16 18:46:20 UTC
Merged, will be 0.2.6.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.