[webkit-reviews] review granted: [Bug 77960] CSS3 calc() - simple parse time evaluation : [Attachment 125806] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 77960: CSS3 calc() - simple parse time evaluation
https://bugs.webkit.org/show_bug.cgi?id=77960

Attachment 125806: Patch
https://bugs.webkit.org/attachment.cgi?id=125806&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125806&action=review


I know it's a pain to post these smaller patches, but these are 1000x easier to
review. :)

> Source/WebCore/css/CSSCalculationValue.cpp:117
> +
> +	   ASSERT_NOT_REACHED();
> +	   return 0;

Does the compiler complain if you leave this out? If not, I'd just delete this
since you will get a warning if a new Calc type is added since your switch
above doesn't have a "default" clause (== a good thing!).

> Source/WebCore/css/CSSCalculationValue.cpp:206
> +	   case CalcMod:
> +	       if (rightValue)
> +		   return static_cast<int>(leftValue) %
static_cast<int>(rightValue);
> +	       return std::numeric_limits<double>::quiet_NaN();

Wasn't mod dropped from the spec?

> Source/WebCore/css/CSSCalculationValue.cpp:209
> +	   ASSERT_NOT_REACHED();
> +	   return 0;

Ditto re: deleting the unreachable code if the compiler doesn't complain.