[webkit-reviews] review granted: [Bug 118481] 30% JSBench regression (caused by adding column numbers to stack traces) : [Attachment 206297] patch 4: addressed feedback comments and attempt to appease EWS bots again.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 118481: 30% JSBench regression (caused by adding column numbers to stack
traces)
https://bugs.webkit.org/show_bug.cgi?id=118481

Attachment 206297: patch 4: addressed feedback comments and attempt to appease
EWS bots again.
https://bugs.webkit.org/attachment.cgi?id=206297&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=206297&action=review


r=me, but I think I spotted two things you should fix before landing.

> Source/JavaScriptCore/ChangeLog:69
> +	   - Also fixed some bugs in the choice of divot positions to use. For
example,
> +	     both Eval and Function expressions previously use column numbers
from
> +	     the start of the expression but use the line number at the end of
the
> +	     expression. This is now fixed to be consistently either using the
start

You've still got some present vs future tense confusion here. I think you meant
"used" instead of "use" in two places here?

"to be consistently either using" is a pretty awkward verb phrase, too.

> Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:39
> +    //   1. FatLine: 	 22-bit line,  8-bit column.
> +    //   2. FatColumn:	  8-bit line, 22-bit column.
> +    //   3. FatLineAndColumn: 32-bit line, 32-bit column.

The reason the style bot complains about this is that it makes the code hard
for other engineers to edit / refactor later. For example, if we ever rename
FatLine, FatColumn, or FatLineAndColumn (as you've already done once in
preparing this patch), there's a lot of extra typing to line all the comments
up again.

You might think that we should just make an exception for the select pieces of
code that you like to line up this way, since there aren't very many of them.
Unfortunately, it's not practical in a large, distributed open source project
to maintain special commenting rules tailored to individual developers.