[Webkit-unassigned] [Bug 41845] refactor identifier parsing in lexer

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63583|review?                     |review+
               Flag|                            |




--- Comment #18 from Darin Adler <darin at apple.com>  2010-08-05 08:21:48 PST ---
(From update of attachment 63583)
Thanks. This looks good.

> +ALWAYS_INLINE JSTokenType Lexer::parseIdent(JSTokenData* lvalp, LexType lexType)

Since this is identifier parsing, how about naming it parseIdentifier? I know the file sometimes does abbreviate to "ident", but I find the full word easier to read as I almost always do in code in general.

> +    while (true) {
> +        if (LIKELY(isIdentPart(m_current)))
> +            shift();
> +        else if (LIKELY(m_current != '\\'))
> +            break;
> +        else {
> +            // \uXXXX unicode characters
> +            bufferRequired = true;
> +            if (identifierStart != currentCharacter())
> +                m_buffer16.append(identifierStart, currentCharacter() - identifierStart);
> +            shift();
> +            if (UNLIKELY(m_current != 'u'))
> +                return ERRORTOK;
> +            shift();
> +            int character = getUnicodeCharacter();
> +            if (UNLIKELY(character == -1))
> +                return ERRORTOK;
> +            if (UNLIKELY(m_buffer16.size() ? !isIdentPart(character) : !isIdentStart(character)))
> +                return ERRORTOK;
> +            record16(character);
> +            identifierStart = currentCharacter();
> +        }
> +    }

I prefer the "early return" style, so if it gives you the same efficiency I'd like this to be written as follows:

    if (LIKELY(isIdentPart(m_current))) {
        shift();
        continue;
    }
    if (LIKELY(m_current != '\\'))
        break;
    ...

With the rest of the loop body not indented.

> +        // keywords are not checked if there was an \uXXXX in the identifier

This is a confusing comment. I think what we mean to say is that keywords must not be recognized in such cases. Saying keywords "are not checked" leaves it ambiguous whether we are describing a bug or a feature.

Also WebKit preferred style is to capitalize comments and end with a period. Especially a comment like this one that is a full sentence.

> +        // Continue to the next case

Would be better to have a period and use the terminology "Fall through" as we do in other similar comments.

> +        ALWAYS_INLINE JSTokenType parseIdent(JSTokenData* lvalp, LexType lexType);

No need for the argument names lvavlp and lexType here. The types of the arguments speak for themselves without argument names and in such cases we omit the argument names in WebKit code.

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