Add debug assertions to monthFromDayInYear() in DateMath.h to validate the `dayInYear` parameter. This has the added benefit of silencing many "Dead nested assignment" issues when running the clang static analyzer.
Created attachment 394867 [details] Patch v1
Comment on attachment 394867 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=394867&action=review Not sure why the static analyzer trusts our assertions > Source/WTF/wtf/DateMath.h:351 > + ASSERT(d < (step + 31)); No need for the parentheses here
(In reply to Darin Adler from comment #2) > Comment on attachment 394867 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394867&action=review > > Not sure why the static analyzer trusts our assertions The static analyzer runs on Debug builds, so by adding this assertion, the value of `step` is read and the analyzer won't issue a warning that `step` is written to but not read from here: if (d < (step += 30)) > > Source/WTF/wtf/DateMath.h:351 > > + ASSERT(d < (step + 31)); > > No need for the parentheses here Will fix when landing.
Hmm...well, it looks like we have tests that end up passing in invalid values: Less than zero: js/date-constructor.html js/date-parse-comments-test.html js/date-parse-test.html More than 364/365: js/date-timeClip-large-values.html
Comment on attachment 394867 [details] Patch v1 I think using UNUSED_VARIABLE() or a similar macro would be better here. Should I make a new macro that has a more explicit name like IGNORE_UNUSED_VARIABLE_WRITE()?
I don’t think UNUSED is how we should fix this. I suggest we change that last one to just: d < step + 30 without the +=. Or ... rewrite to whole thing to not use a local variable at all. Either to use an array to do this rather than writing it out unrolled, or find some other form to write it in that doesn’t involve this "too clever" += mixed in with if statements. After all, these are constants. One neat way to write it would be to write a constexpr function to compute the start day of a month and use that function, passing a month integer instead of having the function compute these values live. Then the looping will be done at compile time, not runtime. There are probably all sorts of other fun ways to write this.
Created attachment 394875 [details] Patch v2
(In reply to Darin Adler from comment #6) > I don’t think UNUSED is how we should fix this. > > I suggest we change that last one to just: > > d < step + 30 > > without the +=. I went with this option for now. Thanks!
Committed r259202: <https://trac.webkit.org/changeset/259202> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394875 [details].
<rdar://problem/61060041>