[Webkit-unassigned] [Bug 16753] date set methods with no args should result in NaN (Acid3 bug)

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=16753


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #19206|review?                     |review-
               Flag|                            |




------- Comment #16 from eric at webkit.org  2008-02-18 22:41 PDT -------
(From update of attachment 19206)
Thanks for the patch!

A few comments.  One, you should familiarize yourself with the WebKit coding
style guidelines:
http://webkit.org/coding/coding-style.html

A few "violations" I see in this patch include:
if (foo)
{

(The { should be on the same line as the if)

and:

if (0 != args.size())

should be:

if (args.size()) (we avoid == 0 and != 0 when the meaning is otherwise clear
from context.  e.g. comparing pointers to NULL or using size() for isEmpty()...
in this case, there migth even be an isEmpty() which could be more clear).

Single line ifs should not have { }

Wouldn't:
ok = !(isinf(millis) || isnan(millis));
be equivalent to:
ok = isfinite(mills); ?

Also, the above if (ok) block, generally we prefer an early-return style in
webkit.  In this instance you would use:
if (!ok)
   return false;

instead of wrapping that section in an if (ok) block.

Likewise, it would be more clear (or at least more inline with webkit style) to
do:

if (!args.size()) {
   result = jsNaN();
   date->setInternalValue(result);
   return result;
} 

as an early return, instead of adding a long if block.

We'll need to of course run SunSpider with this change to make sure there is
not speed regression due to added branching in these functions.

r- for the style issues.

Ideallly your patch would include the diff for the test case, since someone
else will be landing this for you (unless you have commit-bit and I simply
don't recognize your name).

Thanks again for the patch!


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.