[webkit-reviews] review granted: [Bug 84523] Change JavaScript lexer to use 0 instead of -1 for sentinel, eliminating the need to put characters into ints : [Attachment 138227] Patch with fix for Qt build issue

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 84523: Change JavaScript lexer to use 0 instead of -1 for sentinel,
eliminating the need to put characters into ints
https://bugs.webkit.org/show_bug.cgi?id=84523

Attachment 138227: Patch with fix for Qt build issue
https://bugs.webkit.org/attachment.cgi?id=138227&action=review

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


Overall this patch makes me happy, I'd still rather not mix renames in with
semantic/functionality changes, but in this case the overhead is relatively
low.

> Source/JavaScriptCore/parser/Lexer.cpp:3
> + *  Copyright (C) 2006, 2007, 2008, 2009, 2012 Apple Inc. All Rights
Reserved.

Err, this should also probably have 2011 in as well.  I'd swear I've made that
change before and it just keeps disappearing.

> Source/JavaScriptCore/parser/Lexer.cpp:371
> -UString Lexer<T>::getInvalidCharMessage()
> +UString Lexer<T>::invalidCharacterMessage() const

I would rather we didn't have unrelated renaming in patches that change
functionality (albeit slightly), as it makes the patch bigger than it might
otherwise need to be.

> Source/JavaScriptCore/parser/Lexer.cpp:-482
> -    int m_prev = m_current;

/me boggles at that naming

> Source/JavaScriptCore/parser/Lexer.h:3
> + *  Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012 Apple
Inc. All rights reserved.

Can you add 2011 in here as well

> Source/JavaScriptCore/parser/Lexer.h:114
> -	   // Faster than an if-else sequence
> -	   m_current = -1;
>	   if (LIKELY(m_code < m_codeEnd))
>	       m_current = *m_code;
> +	   else
> +	       m_current = 0;

Why the sequencing change? no measurable difference?