[webkit-reviews] review requested: [Bug 106553] [V8] Add IDL Enum support WIP : [Attachment 185173] updated -- final?

[bugzilla-daemon at webkit.org]

Nils Barth <nbarth at google.com> has asked  for review:
Bug 106553: [V8] Add IDL Enum support WIP
https://bugs.webkit.org/show_bug.cgi?id=106553

Attachment 185173: updated -- final?
https://bugs.webkit.org/attachment.cgi?id=185173&action=review

------- Additional Comments from Nils Barth <nbarth at google.com>
(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
		       | ε