[Bug 753455] gstdatetime: gst_date_time_new_from_iso8601_string: Default date is "today"

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=753455

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #309007|none                        |reviewed
             status|                            |

--- Comment #2 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 309007
  --> https://bugzilla.gnome.org/attachment.cgi?id=309007
Patch file

Thanks, looks mostly fine.

We should probably add a comment to the function docs giving some information
on what formats it accepts (beyond just 'the most common formats'), i.e. also
make it explicit that just passing in a time is acceptable as well now.

> 
>-  if (len < 4 || !g_ascii_isdigit (string[0]) || !g_ascii_isdigit (string[1])
>-      || !g_ascii_isdigit (string[2]) || !g_ascii_isdigit (string[3]))
>+  if (len < 4 || !g_ascii_isdigit (string[0]) || !g_ascii_isdigit (string[1]))
>     return NULL;

Here it would be nice to add a comment above that explains what the
expected/allowed strings/formats/things are here (i.e. year or hour?)


>-  ret = sscanf (string, "%04d-%02d-%02d", &year, &month, &day);
>+  if (g_ascii_isdigit (string[2]) && g_ascii_isdigit (string[3])) {

I think we're now not bailing out when there are only 3 valid digits? (maybe
add something to the unit test to check this)

> ymd_hms:
>+  if (year == -1 || month == -1 || day == -1) {
>+    GDateTime *g_date_now_utc, *g_date_now_in_given_tz;

Perhaps rename these to just 'now_utc' and 'now_in_given_tz', otherwise it
looks like glib symbols.


>+    /* No date was supplied: make it today */
>+    g_date_now_utc = g_date_time_new_now_utc ();
>+    if (tzoffset != 0.0) {
>+      /* If a timezone offset was supplied, get the date of that timezone */
>+      gint offset_hours, offset_mins, offset_secs;
>+
>+      offset_hours = (gint) tzoffset;
>+      offset_mins = (gint) (60 * (tzoffset - offset_hours));
>+      offset_secs = (gint) (3600 * (tzoffset - offset_mins));
>+      g_date_now_in_given_tz =
>+          g_date_time_add_hours (g_date_now_utc, offset_hours);
>+      g_date_now_in_given_tz =
>+          g_date_time_add_minutes (g_date_now_in_given_tz, offset_mins);
>+      g_date_now_in_given_tz =
>+          g_date_time_add_seconds (g_date_now_in_given_tz, offset_secs);
>+    } else {
>+      g_date_now_in_given_tz = g_date_now_utc;
>+    }
>+    year = g_date_time_get_year (g_date_now_in_given_tz);
>+    month = g_date_time_get_month (g_date_now_in_given_tz);
>+    day = g_date_time_get_day_of_month (g_date_now_in_given_tz);
>+  }
>   return gst_date_time_new (tzoffset, year, month, day, hour, minute, second);

I think here you might be leaking the two GDateTime created earlier?

Also, did you check already if this code can be simplified e.g. by creating a
GTimeZone or so?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.