[Webkit-unassigned] [Bug 106553] [V8] Add IDL Enum support WIP
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 28 23:31:45 PST 2013
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.
More information about the webkit-unassigned
mailing list