[webkit-reviews] review granted: [Bug 106750] String(new Date(2010, 10, 1)) is wrong in KRAT, YAKT : [Attachment 203026] Fix part 1 v3 - more windows build fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 106750: String(new Date(2010,10,1)) is wrong in KRAT, YAKT
https://bugs.webkit.org/show_bug.cgi?id=106750

Attachment 203026: Fix part 1 v3 - more windows build fix
https://bugs.webkit.org/attachment.cgi?id=203026&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203026&action=review


> Source/JavaScriptCore/ChangeLog:13
> +	   (JSC):

Please remove bogus lines like this one when buggy prepare-ChangeLog adds them.


> Source/JavaScriptCore/ChangeLog:21
> +	   (JSC::VM::VM):

Please remove bogus lines like this one when buggy prepare-ChangeLog adds them.


> Source/JavaScriptCore/runtime/JSDateMath.cpp:135
> +static LocalTimeOffset getLocalTimeOffset(ExecState* exec, double ms)

WebKit coding style would call this localTimeOffset, not getLocalTimeOffset. By
convention, we aim to use get exclusively for functions that use out arguments.


> Source/JavaScriptCore/runtime/JSDateMath.cpp:197
> +    // convert to UTC

Probably should dump this comment. The other function doesn’t have this.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:207
> +    LocalTimeOffset localTimeOffset(false, 0);

Would be nicer to just have the LocalTimeOffset default constructor start with
these values so that the (false, 0) can be omitted.

> Source/JavaScriptCore/runtime/VM.h:107
> +	       : offset(false, 0)

This explicit initializer would not be needed if the default constructor for
LocalTimeOffset existed, and I do suggest adding that. Also, this constructor
sets the offset twice, because the reset function sets the offset.

> Source/JavaScriptCore/runtime/VM.h:114
> +	       offset = LocalTimeOffset(false, 0);

It would be more readable to reset the two members individually.

If you do want to use struct assignment, I’d suggest using the default
constructor here instead of the “false, 0”.

I’m not sure what members the reset function needs to affect; are these ever
inspected without writing values into them? It seems that to invalidate the
cache, only the key would need to be overwritten.

> Source/WTF/ChangeLog:13
> +	   (WTF):

More bogus prepare-ChangeLog unhappiness.

> Source/WTF/ChangeLog:28
> +	   (WTF):

More.

> Source/WTF/wtf/DateMath.cpp:473
> +    else if (localTimeSeconds < 0) // Go ahead a day to make localtime work
(does not work with 0)

Would be idiomatic to put a period on this comment.

> Source/WTF/wtf/DateMath.cpp:475
> +    // FIXME: time_t has a potential problem in 2038

Would be idiomatic to put a period on this comment.

> Source/WTF/wtf/DateMath.cpp:476
> +    time_t localTime = static_cast<time_t>(localTimeSeconds);

Seems unnecessary to involve time_t at all in the !HAVE(TM_GMTOFF) case; can we
isolate this type cast to that side of the #if branch?

> Source/WTF/wtf/DateMath.h:102
> +struct LocalTimeOffset {

It would be better to have the struct at the top of the file rather than right
before the function that returns it.

> Source/WTF/wtf/DateMath.h:103
> +    LocalTimeOffset(bool isDST, int offset)

Could just put "= false" and "= 0" here and make this a default constructor
too.

> Source/WTF/wtf/DateMath.h:109
> +    bool operator==(const LocalTimeOffset& other)

Typically we do != as well as ==, but I suppose it would be dead code.

> Source/WTF/wtf/DateMath.h:118
> +WTF_EXPORT_PRIVATE LocalTimeOffset calculateLocalTimeOffset(double);

Needs an argument name. I suggest “localTimeInMilliseconds” if that is
accurate.