[webkit-reviews] review denied: [Bug 4921] \u escape sequences in JavaScript identifiers : [Attachment 4309] Darin's patch extracted from 4885. Fixes unicode escapes

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Kimmo Kinnunen
<kimmo.t.kinnunen at nokia.com>'s request for review:
Bug 4921: \u escape sequences in JavaScript identifiers
http://bugzilla.opendarwin.org/show_bug.cgi?id=4921

Attachment 4309: Darin's patch extracted from 4885. Fixes unicode escapes
http://bugzilla.opendarwin.org/attachment.cgi?id=4309&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks great (not sure if I can review my own patch).

Where does the specification say that if you use \u sequences, then an
identifier is not a keyword? I re-read it and that was not clear to me. Did you
test other implementations to see if that was so? (Part of the reason I ask is
that I know in C++ it does not work this way; you can use the sequences in
keywords.)

The boolean identifierIsNotToken is a great way to implement that rule, but I
recommend naming it identifierIsNotKeyword instead. Another way to implement it
would be to create a separate state. The old one could be renamed
IdentifierOrKeyword and the new state would be just Identifier. The
IdentifierOrKeyword case could fall into the Identifier case after checking for
and handling keywords.

Besides allowing \u sequences in identifiers, this patch also implements the
full rule for what characters are allowed in identifiers. So the attached test
case is insufficient. We need test cases that cover a variety of valid and
invalid identifiers, as well as covering the case of an identifer that uses a
\u sequence to avoid collision with a keyword.

The minimum that would be needed on this patch would be renaming
identifierIsNotToken to identifierIsNotKeyword and adding more test cases.