[Webkit-unassigned] [Bug 106553] [V8] Add IDL Enum support WIP

[bugzilla-daemon at webkit.org]

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


Nils Barth <nbarth at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #185136|0                           |1
        is obsolete|                            |
 Attachment #185173|                            |review?
               Flag|                            |




--- Comment #14 from Nils Barth <nbarth at google.com>  2013-01-28 23:33:44 PST ---
Created an attachment (id=185173)
 --> (https://bugs.webkit.org/attachment.cgi?id=185173&action=review)
updated -- final?

(In reply to comment #13)
> Looks almost OK. If you want to have the patch be reviewed, please set r?.

Marked as r? -- may have a tweak or two left (a comment or two).

Removed XMLHttpRequest.idl and layout test for now (blocked by JSC etc. fixes).
Updated ChangeLog.

> > Source/WebCore/bindings/scripts/IDLParser.pm:272
> > +    my $unquotedString;
> > +    ($unquotedString) = $quotedString =~ /^"([^"]*)"$/;
> > +    die "Failed to parse string \"" . $quotedString . "\" at " . $self->{Line} if not defined $unquotedString;
> > +    return $unquotedString;
> 
> In code generators we normally write:
> 
>   if ($quotedString =~ /^"(.*)"$/) {
>     return $1;
>   }
>   die ...;

Oops! =P Fixed.

> > Source/WebCore/bindings/scripts/IDLParser.pm:656
> > +    my $numberOfExtendedAttributes = keys %{$extendedAttributeList};
> > +    die "Extended attributes are not applicable to enumerations, but " . $numberOfExtendedAttributes . " present at " . $self->{Line} if $numberOfExtendedAttributes;
> 
> I don't think this check is needed. (If you want to insert this kind of checks, you have to insert more checks, which will mess up the code.)

Understood -- removed. (We die on syntax errors; we're not doing semantic checks here.)

Q: I've added a comment to the code ( sub parseEnum ) that we're ignoring the attributes,
since currently the variable is set and ignored (which raises an eyebrow).
Options include:
0. Ignore with comment (patch)
1. Ignored without comment (current)
2. Just not shift it (delete the my $extendedAttributeList = shift; line) -- then it's dropped earlier
3. Not pass it in at the sub parseDefinition level
        return $self->parseEnum($extendedAttributeList);
        return $self->parseEnum();
Any preferences?
3 is perhaps clearest since by visibly *not* passing it we're saying it's not used
(surrounding lines pass it) so no comment is needed, but I understand if we want to drop it somewhere else or have identical code to other parse(SomeDefinition).


> > Source/WebCore/bindings/scripts/IDLParser.pm:687
> > +    # value list must be non-empty
> 
> Really? (I'm not sure. Please check the spec.)

Yup -- "non-empty" follows from the grammar.
(EnumValueList is non-empty, but the tail/cdr EnumValues can be empty.)
The current parser code is correct, but since this is potentially confusing I added a note.
(Fine to remove if not esp. useful.)

The spec would be clearer with a note in the text; it currently reads:
"The enumeration values are specified as a comma-separated list of string literals. The list of enumeration values must not include duplicates."
It would be clearer written something like:
"The enumeration values are specified as a comma-separated list of string literals. The list of enumeration values *must not be empty and* must not include duplicates."
(Non-empty is syntax, no duplicates is semantics, right?)

http://www.w3.org/TR/WebIDL/#idl-enums
http://www.w3.org/TR/WebIDL/#idl-grammar
[20] Enum          → "enum" identifier "{" EnumValueList "}" ";"
[21] EnumValueList → string EnumValues
[22] EnumValues    → "," string EnumValues 
                      | ε

Note that we don't have:
*[21] EnumValueList → string EnumValues
                       | ε

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