[webkit-reviews] review denied: [Bug 9651] MSVCRT time functions are too strict : [Attachment 9383] Custom implementaion of time functions in WTF, not inlined 2

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 9651: MSVCRT time functions are too strict
http://bugzilla.opendarwin.org/show_bug.cgi?id=9651

Attachment 9383: Custom implementaion of time functions in WTF, not inlined 2
http://bugzilla.opendarwin.org/attachment.cgi?id=9383&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    int dfy = dayFromYear((int)year);

no need for type cast

+    if (day < dfy) {
+	 // The first day of year is beyond day, so we have right year + 1
+	 year -= 1;
     }

no need for braces

-  static double time_tMin = (time_tIsSigned ? - (double)(1ULL << (8 *
sizeof(time_t) - 1)) : 0);
+  if (secs < 0 || secs > time_tMax) {

If this is removed, so that mean that time_tIsSigned is not needed any more?

-  char timebuffer[bufsize];
+  wchar_t timebuffer[bufsize];

What's the rationale for this change?

+      size_t len = WTF::wcsftime(timebuffer, bufsize, L"%c", &t);
+      return jsString(UString((UChar*)timebuffer, len));

This assumes that wchar_t is the same as UChar, but that's not going to be true
on platforms other than Windows.

+	 struct tm t3;

Should be just "tm", not "struct tm", right?

+/*
+*/

What is that empty comment about?

+inline bool getTimeZoneInfo() {
+inline SYSTEMTIME tmToSystemTime(const struct tm& tm) {

We put braces on the next line when defining functions.

+    //FIXME: Cache the result per thread

We put spaces after // comments.

+inline bool operator >=(const struct tm& a, const struct tm& b)
+{
+    bool res = false;
+    if (a.tm_year == b.tm_year) {
+	 if (a.tm_mon > b.tm_mon) {
+	     res = true;
+	 } else if (a.tm_mon == b.tm_mon) {
+	     if (a.tm_mday > b.tm_mday) {
+		 res = true;
+	     } else if (a.tm_mday == b.tm_mday) {
+		 if (a.tm_hour > b.tm_hour) {
+		     res = true;
+		 } else if (a.tm_hour == b.tm_hour) {
+		     if (a.tm_min >= b.tm_min) {
+			 res = true;
+		     }
+		 }
+	     }
+	 }
+    }
+    return res;

Would read a lot better if it had a few more return statements in it.

+static inline tm* gmtime(const time_t* timer) { return ::gmtime(timer); }
+static inline tm* localtime(const time_t* timer) { return ::localtime(timer);
}
+static inline time_t mktime(tm* time) { return ::mktime(time); }
+static inline size_t wcsftime(wchar_t* dest, size_t maxsize, const wchar_t*
format, const tm* timeptr) { return ::wcsftime(dest, maxsize, format, timeptr);
}

These should not have "static" in front of them -- just inline.

+#ifndef _DATEEXTRAS_H_

Should not use a leading underscore -- that's reserved for the compiler and
library.

+#if HAVE(SYS_TIME_H)
+#if PLATFORM(WIN)

Need to include "Platform.h" first if a header file is going to use these
macros.