[webkit-reviews] review denied: [Bug 62613] No context for javascript parse errors. : [Attachment 97522] proposed patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Juan C. Montemayor
<jmont at apple.com>'s request for review:
Bug 62613: No context for javascript parse errors.
https://bugs.webkit.org/show_bug.cgi?id=62613

Attachment 97522: proposed patch
https://bugs.webkit.org/attachment.cgi?id=97522&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97522&action=review

Almost there, just a few style issues remain (i'm only complaining about the
other whitespace issues because the style errors will require you to update
your patch anyway).  The only real problem is the additional branches added for
eval/arguments in parseUnaryExpression (see my comments below)

Make sure that all the tests that don't fail before this patch still pass after
it (i'm surprised that there aren't any non-js/sputnik tests that ended up
testing parse error messages).

If you use "webkit-patch post" the style checker will run locally and catch any
other commit-queue breaking style issues.

> Source/JavaScriptCore/parser/JSParser.cpp:85
> +    JSParser(Lexer*, JSGlobalData*, FunctionParameters*, bool
isStrictContext, bool isFunction,  const SourceCode*);

Yay style error! 1 space not two (the commit queue barfs on all style errors)

> Source/JavaScriptCore/parser/JSParser.cpp:349
> +    ALWAYS_INLINE void updateErrorMessageSpecialCase(JSTokenType
expectedToken) 
> +    {

This function should be doing early returns rather than assign->break->return

> Source/JavaScriptCore/parser/JSParser.cpp:425
> +	   m_errorMessage = UString(msg);

This UString conversion should happen automatically

> Source/JavaScriptCore/parser/JSParser.cpp:512
> -
> +    

Adding whitespace

> Source/JavaScriptCore/parser/JSParser.cpp:933
> +    

Added whitespace (alas xcode will fight to insert this whitespace :-/ )

> Source/JavaScriptCore/parser/JSParser.cpp:1503
> +	   failIfFalseIfStrictWithMessage(m_statementDepth == 1, "Function
declarations cannot be in a nested block in strict mode");

You should ask geoff if he can come up with a better worded sentence than this
-- it's somewhat clunky as is.

> Source/JavaScriptCore/parser/JSParser.cpp:1586
> +	   failIfTrueWithMessage(*name ==
m_globalData->propertyNames->underscoreProto, "Can't name a function
__proto__");

Should use Cannot or Can't consistently -- i'd lean towards Cannot

> Source/JavaScriptCore/parser/JSParser.cpp:2386
> -    bool isEvalOrArguments = false;
> +    bool isEval = false;
> +    bool isArguments = false;
>      if (strictMode() && !m_syntaxAlreadyValidated) {
>	   if (context.isResolve(expr)) {
> -	       isEvalOrArguments = *m_lastIdentifier ==
m_globalData->propertyNames->eval || *m_lastIdentifier ==
m_globalData->propertyNames->arguments;
> +	       isEval = *m_lastIdentifier == m_globalData->propertyNames->eval;

> +	       isArguments = *m_lastIdentifier ==
m_globalData->propertyNames->arguments;
>	   }
>      }
> -    failIfTrueIfStrict(isEvalOrArguments && modifiesExpr);
> +    failIfTrueIfStrictWithMessage(isEval && modifiesExpr, "'eval' cannot be
modified in strict mode");
> +    failIfTrueIfStrictWithMessage(isArguments && modifiesExpr, "'arguments'
cannot be modified in strict mode");

These changes are unnecessary as you can pull the term 'eval' or 'arguments'
out of m_lastIdentifier (we try not to add extra branches to valid code paths
of the parser and lexer)

> Source/JavaScriptCore/parser/JSParser.cpp:2394
> +	   failIfTrueIfStrictWithMessage(isEval, "'eval' cannot be modified in
strict mode");
> +	   failIfTrueIfStrictWithMessage(isArguments, "'arguments' cannot be
modified in strict mode");

as above

> Source/JavaScriptCore/parser/JSParser.cpp:2404
> +	   failIfTrueIfStrictWithMessage(isEval, "'eval' cannot be modified in
strict mode");
> +	   failIfTrueIfStrictWithMessage(isArguments, "'arguments' cannot be
modified in strict mode");

And again

> Source/JavaScriptCore/parser/JSParser.h:37
> +class SourceCode;   

Added whitespace

> Source/JavaScriptCore/parser/JSParser.h:141
> +	   

Moar whitespace