Bug 209718 – Fix "Dead nested assignment" static analyzer warning in monthFromDayInYear()

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 394867 [details]
Patch v1

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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()?

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 394875 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(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!

[EWS]

Committed r259202: <https://trac.webkit.org/changeset/259202>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394875 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61060041>