[Webkit-unassigned] [Bug 62613] No context for javascript parse errors.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62613


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97522|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #15 from Oliver Hunt <oliver at apple.com>  2011-06-16 17:52:41 PST ---
(From update of attachment 97522)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.