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

[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 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 Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217262&action=review


r=me

Much improved. I think this is ready to land, with some fixups below.

> Source/JavaScriptCore/ChangeLog:18
> +	      FunctionExecutables. This has been fixed so that they all use
base-0
> +	      columns. When the executable gets linked, the column is adjusted
into
> +	      a base-1 value.

Why should unlinked and linked executables disagree on the base used for column
counting?

> Source/JavaScriptCore/ChangeLog:42
> +	      The second case is needed so that we can add an appropriate to
the end

Missing a word after "appropriate".

> Source/JavaScriptCore/ChangeLog:52
> +	      This interpretation is janky, but was present before this patch.
We can
> +	      clean that up later in a nother patch.

"another"

> Source/JavaScriptCore/parser/Parser.h:417
> +    JSTextPosition positionBeforeLastLineFeed() const { return
m_lexer->positionBeforeLastLineFeed(); }

We idiomatically call these "newlines" -- as in "positionBeforeLastNewline" --
and not line feeds. "\n" is the newline character.

> Source/JavaScriptCore/runtime/Executable.h:569
> +    static FunctionExecutable* create(VM& vm, const SourceCode& source,
UnlinkedFunctionExecutable* unlinkedExecutable, unsigned firstLine, unsigned
lastLine, unsigned startColumn, unsigned endColumn, bool bodyExcludesBraces =
false)

"excludes = false", in English, would be stated as "does not not include".
Double negatives are notoriously hard for humans to read, even though computers
can handle them just fine. Better to name this variable "bodyIncludesBraces",
and invert the code.

> Source/WebCore/testing/Internals.cpp:1301
> +

Stray newline.

> Source/WebCore/testing/Internals.cpp:1305
> +

Stray newline.

> Source/WebCore/testing/Internals.cpp:1329
> +    } else {
> +	   if (executable->isEvalExecutable())
> +	       result.append("eval");
> +	   else {
> +	       ASSERT(executable->isProgramExecutable());
> +	       result.append("program");
> +	   }
> +    }

Using "else if" here will reduce the indentation.

> Source/WebCore/testing/Internals.idl:141
> +    // Calling scriptBoundsFor() with null gets the bounds for the script of
the current scope.

This comment should mention that you can call with no arguments. That's the
preferred way to get the metadata for the current scope. This comment should
also say "metadata", to match the function name.