[webkit-reviews] review requested: [Bug 124245] Add tracking of endColumn for Executables : [Attachment 217262] patch 6: addressed all feedback + many bug fixes.

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has asked  for review:
Bug 124245: Add tracking of endColumn for Executables
https://bugs.webkit.org/show_bug.cgi?id=124245

Attachment 217262: patch 6: addressed all feedback + many bug fixes.
https://bugs.webkit.org/attachment.cgi?id=217262&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
This column computation work is turning out to be more complex than the
bytecode level breakpoint patch that depends on it.  Anyway, the patch looks
much better now and we have more tests to prove that it works.

(In reply to comment #17)
> (1) I think this would be more clearly expressed as:
m_functionEndColumn(m_lineCount ? node->endColumn() : node->endColumn() -
node->startColumn()). It's FunctionBodyNode that holds the unrelocatable data,
so it's clearer to express fixup in terms of it.

I agree, but I ended up renaming m_functionStart/EndColumn to
m_unlinkedBodyStart/EndColumn to distinguish it from
m_unlinkedFunctionNameStart which points to the start of the function name
instead.

> (2) I would rename "function*" to "unlinked*". So, "m_unlinkedEndColumn".
That helps to express why we're doing this transformation, and why we do
inverse transformation in CodeBlock.

Done.

> (3) Why don't we need to fix up m_functionStartColumn? In a context-free
relocatable format, I would expect the start column to be 0, or perhaps just
the offset between "f" and "{" in "function f() { ...". In other words, what
happens in this case?:
> 
>      <script>function f() { }</script>
> <script>function f() { }</script>
> 
> The first function starts on column 14, and the second on column 9. The cache
will hold the UnlinkedFunctionExecutable from the first function. Will the
second function report that it starts on column 14?

Previously, both <script> tag cases are reported as starting at column 1 where
column 1 is the char immediately following the <script> tag.  This is similar
to how eval code is handled.  However, it is unsatisfying and we can do better
(as you expected in your comment).  This is now fixed.	The handling of this
case is in UnlinkedFunctionExecutable::link() which now adjusts startColumn
accordingly.  I've also added test cases to the new test that exercises this
code.

> > Source/JavaScriptCore/runtime/Executable.h:366
> > +	     , m_endColumn(UINT_MAX)
> 
> I still think it's weird to initialize and test m_endColumn with sentinel
value, but not m_startColumn.

Agreed.  m_startColumn is now initialized to UINT_MAX as well, and asserted
accordingly at all the appropriate places.

Previously, I also said that I would look into increasing the symmetry of the
way we compute startColumn and endColumn.  After revisiting all the cases, I
decided that:
1. It doesn't make sense to store the endColumn in SourceCode because the
endColumn is computed by the lexer / parser after SourceCode is instantiated.
2. It also doesn't make sense to go back and update SourceCode with an
appropriate endColumn value post-parsing.  This is because the SourceCode
instance used for a subsequent linkage of the UnlinkedExecutable may be
different anyway.  The best place to store the endColumn is still in the
UnlinkedExecutable along with the start line, last line, and startColumn that
were already there.

That said, I did re-factored the code to be more symmetrical in the handling of
startColumn and endColumn.  Now, when a class / struct has an endColumn, it
also has a startColumn.  However, in the case of UnlinkedCodeBlock and
EvalNode, m_startColumn is always a constant 0.