date_parseString has a "syntax" label that sets *result to 0 and returns false. The callers don't use the result if we return false, so we can just "return false;" instead of "goto syntax;".

Various simple changes to make the code more readable and conform better to our style guide. date_parseString is incredibly complicated. I'd love to refactor it more but it'd take a while to figure out which cases it's handling where and I can't really justify working on it more. These simple patches make the code a lot nicer though.

Comment on attachment 8435820 [details] [diff] [review] Part 3 - More date_parseString cleanup Review of attachment 8435820 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +965,5 @@ > + /* > + * Allow TZA before the year, so 'Wed Nov 05 21:49:11 GMT-0800 1997' > + * works. > + * > + * Uses of seenPlusMinus allow : in TZA, so Java no-timezone style I'd put single quotes around the ':' because I had to read this sentence twice to understand what it was saying. @@ +977,5 @@ > /* offset */ > if (n < 24) > n = n * 60; /* EG. "GMT-3" */ > else > n = n % 100 + n / 100 * 60; /* eg "GMT-0430" */ This is a candidate for ?: usage, if you feel inclined. @@ +1150,1 @@ > return false; I'd suggest reverting this change. There are lots of cases above where there are one or more |if|s and the final |else| does |return false|, so even though you've avoided some indentation you've moved away from that pattern. @@ +1158,1 @@ > return false; Ditto.

Some similar changes to date_parseISOString.

Comment on attachment 8435834 [details] [diff] [review] Part 4 - Cleanup date_parseISOString Review of attachment 8435834 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +789,5 @@ > #define PEEK(ch) (i < limit && s[i] == ch) > > +#define NEED(ch) \ > + JS_BEGIN_MACRO \ > + if (i >= limit || s[i] != ch) { return false; } else { ++i; } \ It's best to put the '\' in the 79th char. That way you don't have to change every line when the longest line's length changes in the future. Ditto for the macros below, while you're here. Also: JS_{BEGIN,END}_MACRO are still lingering, huh? On Windows JS_END_MACRO has some warning stuff, but we have numerous places where we just use |do { ... } while (0)| directly in macros, so I'm happy if you want to do that here.

date_regionMatches's ignoreCase argument is always |1|, so I removed the argument and the "if (ignoreCase)" check. I also renamed some functions to better match our style: date_parse -> ParseDate, date_RegionMatches -> RegionMatches etc. The test does what your suggested in another bug: it calls Date.parse with Latin1 and TwoByte strings and compares the output.

Comment on attachment 8436031 [details] [diff] [review] Part 5 - Handle Latin1 strings Review of attachment 8436031 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +568,2 @@ > > return result; Just realized we can remove "bool result" and instead: return count == 0; Fixed locally.

Comment on attachment 8436031 [details] [diff] [review] Part 5 - Handle Latin1 strings Review of attachment 8436031 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +546,5 @@ > 10000 + 7 * 60, 10000 + 6 * 60, /* MST/MDT */ > 10000 + 8 * 60, 10000 + 7 * 60 /* PST/PDT */ > }; > > +/* Helper for ParseDate. */ I'd just remove that comment. It doesn't add anything useful. @@ +788,1 @@ > JS_END_MACRO I'd be ok with removing the JS_{BEGIN,END}_MACRO here and in the following macros.

https://hg.mozilla.org/integration/mozilla-inbound/rev/79bd5ad8f318 https://hg.mozilla.org/integration/mozilla-inbound/rev/9716bdb2ff5f https://hg.mozilla.org/integration/mozilla-inbound/rev/4c72b5930984 https://hg.mozilla.org/integration/mozilla-inbound/rev/6b73fec9b7d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/64553c483cd1 (In reply to Nicholas Nethercote [:njn] from comment #4) > I'd put single quotes around the ':' because I had to read this sentence > twice to understand what it was saying. Done. > > if (n < 24) > > n = n * 60; /* EG. "GMT-3" */ > > else > > n = n % 100 + n / 100 * 60; /* eg "GMT-0430" */ > > This is a candidate for ?: usage, if you feel inclined. I left it as if-else; it's pretty complicated and I find if-else to be clearer in such cases. > I'd suggest reverting this change. There are lots of cases above where there > are one or more |if|s and the final |else| does |return false|, so even > though you've avoided some indentation you've moved away from that pattern. Done. (In reply to Nicholas Nethercote [:njn] from comment #6) > It's best to put the '\' in the 79th char. That way you don't have to change > every line when the longest line's length changes in the future. Ditto for > the macros below, while you're here. > > Also: JS_{BEGIN,END}_MACRO are still lingering, huh? On Windows JS_END_MACRO > has some warning stuff, but we have numerous places where we just use |do { > ... } while (0)| directly in macros, so I'm happy if you want to do that > here. Done as part of part 5. (In reply to Nicholas Nethercote [:njn] from comment #9) > I'd just remove that comment. It doesn't add anything useful. Done. > I'd be ok with removing the JS_{BEGIN,END}_MACRO here and in the following > macros. Done.

https://hg.mozilla.org/mozilla-central/rev/79bd5ad8f318 https://hg.mozilla.org/mozilla-central/rev/9716bdb2ff5f https://hg.mozilla.org/mozilla-central/rev/4c72b5930984 https://hg.mozilla.org/mozilla-central/rev/6b73fec9b7d4 https://hg.mozilla.org/mozilla-central/rev/64553c483cd1