| Summary: | Look into possibilities of typedef in webkit idl files | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||||||||
| Component: | WebGL | Assignee: | Andrey Adaikin <aandrey> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aandrey, abarth, antonm, arv, cmarrin, dglazkov, enne, eric, haraken, japhet, kbr, loislo, nbarth, ojan.autocc, ojan, pfeldman, rniwa, tonikitoo, webkit.review.bot, yurys | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 109060 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Zhenyao Mo
2011-01-12 17:15:17 PST
Created attachment 186866 [details]
Patch
Comment on attachment 186866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186866&action=review This patch is very exciting; it would be a huge cleanup to be able to use typedefs in WebGL's IDL for example. However, I perceive a few problems with this implementation. Perhaps others who work on the bindings will have more and better suggestions, but there are at least a couple of things that I definitely think need to be cleaned up. > Source/WebCore/bindings/scripts/IDLParser.pm:102 > +struct( ExtendedType => { The name of this class is not clear. It actually represents either a type or a typedef. Also, for consistency, it should probably not start with a capital letter. > Source/WebCore/bindings/scripts/IDLParser.pm:104 > + type => '$', # Type of typedef This comment is not clear. Please change it to something like: "If this represents a typedef, the actual type it refers to." > Source/WebCore/bindings/scripts/IDLParser.pm:166 > + my $msg = "Unexpected ExtendedAttributeList in typedef \"$name\" at " . $self->{Line}; For consistency I think this should read "extendedAttributeList" (no initial capital letter). > Source/WebCore/bindings/scripts/IDLParser.pm:749 > + my $extendedAttributeList = shift; # ignored: Extended attributes are not applicable to typedefs themselves Then please change the call sites to no longer pass this argument, leaving this comment here though. > Source/WebCore/bindings/scripts/IDLParser.pm:754 > + my $typedef = $self->parseExtendedType(); I strongly think you should call assertNoExtendedAttributesInTypedef here so that errors are caught when the typedef is parsed, not when it's used. > Source/WebCore/bindings/scripts/IDLParser.pm:759 > + die "typedef redifinition for " . $name . " at " . $self->{Line} if exists $typedefs{$name}; typo: redifinition -> redefinition > Source/WebCore/bindings/scripts/IDLParser.pm:1066 > + $extendedAttributeList = $typedef->extendedAttributes(); Is this correct? Won't this override any extended attributes for the attribute with those for its type? > Source/WebCore/bindings/scripts/IDLParser.pm:1349 > + $paramDataNode->extendedAttributes($typedef->extendedAttributes()); Same question about whether overriding the domSignature's extendedAttributes with those coming from the parsed type is correct. > Source/WebCore/bindings/scripts/IDLParser.pm:1673 > +sub parseExtendedType I think a name like "parseTypeOrTypedef" would be clearer. > Source/WebCore/bindings/scripts/IDLParser.pm:1679 > + $extendedAttributeList = $self->parseExtendedAttributeListAllowEmpty(); Is this really correct? We only parse extended attributes on a type if there aren't any specified e.g. on the attribute or operation? This seems wrong. > Source/WebCore/bindings/scripts/IDLParser.pm:1690 > + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); As mentioned above, I think this check should go inside parseTypedef. > Source/WebCore/bindings/scripts/IDLParser.pm:1787 > + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); Again, this check should be done earlier. Is this part of WebIDL (mostly curious). I assume the goal is to replace all the "unsigned int" usage in WebGLRenderingContext? (In reply to comment #3) > Is this part of WebIDL (mostly curious). Yes, Web IDL supports typedefs. > I assume the goal is to replace all the "unsigned int" usage in WebGLRenderingContext? That would be one goal. It would make the WebKit IDL for WebGL much closer to the real Web IDL. OK. Sounds fantastic. Comment on attachment 186866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186866&action=review Thanks for the review. >> Source/WebCore/bindings/scripts/IDLParser.pm:102 >> +struct( ExtendedType => { > > The name of this class is not clear. It actually represents either a type or a typedef. Also, for consistency, it should probably not start with a capital letter. Generally speaking it has nothing to do with a typedef. This (helper) class is used to represent a parsed result of the WebIDL grammar "ExtendedAttributeList Type". The typedef is just a binding from a string name to an ExtendedType. AFAIU, all exported types start with a lowercase letter (like "idlDocument", "domInterface"), whereas types used internally with a capital letter (like "Token"). Is my understanding correct? I introduce an internal type. >> Source/WebCore/bindings/scripts/IDLParser.pm:104 >> + type => '$', # Type of typedef > > This comment is not clear. Please change it to something like: "If this represents a typedef, the actual type it refers to." Sorry about that. I changed it to "# Type of data". >> Source/WebCore/bindings/scripts/IDLParser.pm:166 >> + my $msg = "Unexpected ExtendedAttributeList in typedef \"$name\" at " . $self->{Line}; > > For consistency I think this should read "extendedAttributeList" (no initial capital letter). done. >> Source/WebCore/bindings/scripts/IDLParser.pm:749 >> + my $extendedAttributeList = shift; # ignored: Extended attributes are not applicable to typedefs themselves > > Then please change the call sites to no longer pass this argument, leaving this comment here though. For consistency I think this should stay as is, like in parseEnum subroutine (I stole this comment from there) and others. >> Source/WebCore/bindings/scripts/IDLParser.pm:754 >> + my $typedef = $self->parseExtendedType(); > > I strongly think you should call assertNoExtendedAttributesInTypedef here so that errors are caught when the typedef is parsed, not when it's used. But we do want an extendedAttribute here, if provided. The WebIDL grammar of typedef: > ExtendedAttributeList "typedef" ExtendedAttributeList Type identifier ";" The first ExtendedAttributeList is to be ignored. The second one is a part of typedef. For example: > typedef [Clamp] unsigned int CLAMPED_UINT; >> Source/WebCore/bindings/scripts/IDLParser.pm:759 >> + die "typedef redifinition for " . $name . " at " . $self->{Line} if exists $typedefs{$name}; > > typo: redifinition -> redefinition done. >> Source/WebCore/bindings/scripts/IDLParser.pm:1066 >> + $extendedAttributeList = $typedef->extendedAttributes(); > > Is this correct? Won't this override any extended attributes for the attribute with those for its type? Yes, as far as I can see. We should parse an ExtendedType here instead of a type. If $extendedAttributeList is not empty it will be never overridden. >> Source/WebCore/bindings/scripts/IDLParser.pm:1673 >> +sub parseExtendedType > > I think a name like "parseTypeOrTypedef" would be clearer. Renamed to "parseTypeOrExtendedType". >> Source/WebCore/bindings/scripts/IDLParser.pm:1679 >> + $extendedAttributeList = $self->parseExtendedAttributeListAllowEmpty(); > > Is this really correct? We only parse extended attributes on a type if there aren't any specified e.g. on the attribute or operation? This seems wrong. Yes, as far as I can see. At this point we expect 0 or 1 ExtendedAttributeList. The $extendedAttributeList passed in tells us if we already have parsed an ExtendedAttributeList. >> Source/WebCore/bindings/scripts/IDLParser.pm:1690 >> + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); > > As mentioned above, I think this check should go inside parseTypedef. Here we protect from the following: typedef [Clamp] unsigned int CLAMPED_UINT; ... void func(in [Optional] CLAMPED_UINT x); // should die, equal to: void func(in [Optional] [Clamp] unsigned int x); Quote from the WebIDL spec (http://www.w3.org/TR/WebIDL/#idl-typedefs): "A typedef must not be used as the type of a construct if an extended attribute that appears after the typedef keyword would be disallowed from appearing directly on that construct." >> Source/WebCore/bindings/scripts/IDLParser.pm:1787 >> + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); > > Again, this check should be done earlier. Here we still want to support typedefs, but an ExtendedAttribute is not appropriate (was already parsed). Created attachment 187006 [details]
Patch
review anyone please? Comment on attachment 187006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187006&action=review > Source/WebCore/bindings/scripts/IDLParser.pm:102 > +struct( ExtendedType => { Maybe 'DefinedType' would be a better wording? > Source/WebCore/bindings/scripts/IDLParser.pm:1674 > +sub parseTypeOrExtendedType What is this parsing in the IDL grammer (http://www.w3.org/TR/WebIDL/#idl-grammar)? We'd like to make the parser as much as conformant with the IDL grammer. > Source/WebCore/bindings/scripts/test/TestCallback.idl:32 > +typedef Class2 TYPEDEF_CLASS2; Would you add new tests for 'typedef' instead of replacing existing tests? Replacing existing tests makes the intention of the existing tests unclear. Comment on attachment 186866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186866&action=review >>> Source/WebCore/bindings/scripts/IDLParser.pm:754 >>> + my $typedef = $self->parseExtendedType(); >> >> I strongly think you should call assertNoExtendedAttributesInTypedef here so that errors are caught when the typedef is parsed, not when it's used. > > But we do want an extendedAttribute here, if provided. The WebIDL grammar of typedef: I see, yes, extended attributes should be allowed here. However, I don't agree with your reading of the Web IDL grammar for typedef. Typedef itself doesn't have an ExtendedAttributeList preceding it. The initial ExtendedAttributeList you describe is associated with grammar rules like Definitions, Argument, etc. The grammar for Typedef is: "typedef" ExtendedAttributeList Type identifier ";" I also don't agree that for example the Argument's ExtendedAttributeList is simply ignored. As far as I can tell from reading the Web IDL spec, if an extended attribute is specified for an argument, and the argument's type is also a typedef, then the extended attributes applied are the combination of those two lists. If it turns out that the two lists contain incompatible extended attributes, then an error is raised. >>> Source/WebCore/bindings/scripts/IDLParser.pm:1679 >>> + $extendedAttributeList = $self->parseExtendedAttributeListAllowEmpty(); >> >> Is this really correct? We only parse extended attributes on a type if there aren't any specified e.g. on the attribute or operation? This seems wrong. > > Yes, as far as I can see. At this point we expect 0 or 1 ExtendedAttributeList. The $extendedAttributeList passed in tells us if we already have parsed an ExtendedAttributeList. The only situation where we call this and an extended attribute list is allowed is if we're parsing a typedef. Otherwise the extended attribute list has already been parsed and is associated with another grammar rule like Argument. >>> Source/WebCore/bindings/scripts/IDLParser.pm:1690 >>> + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); >> >> As mentioned above, I think this check should go inside parseTypedef. > > Here we protect from the following: > > typedef [Clamp] unsigned int CLAMPED_UINT; > ... > void func(in [Optional] CLAMPED_UINT x); // should die, equal to: void func(in [Optional] [Clamp] unsigned int x); > > Quote from the WebIDL spec (http://www.w3.org/TR/WebIDL/#idl-typedefs): > "A typedef must not be used as the type of a construct if an extended attribute that appears after the typedef keyword would be disallowed from appearing directly on that construct." OK. I agree that this check is in the right place, but it's checking the wrong thing. It should be assembling the combined extended attributes from e.g. the Argument and the typedef and then checking the whole set for incompatible extended attributes. >>> Source/WebCore/bindings/scripts/IDLParser.pm:1787 >>> + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); >> >> Again, this check should be done earlier. > > Here we still want to support typedefs, but an ExtendedAttribute is not appropriate (was already parsed). Extended attributes can still be applied to arguments even if the argument's type is a typedef. Comment on attachment 187006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187006&action=review As I'm studying this patch and the Web IDL grammar more, it's becoming clearer to me that this patch is not correct as written. Please look at the comments above and correct any misunderstandings I have. >> Source/WebCore/bindings/scripts/IDLParser.pm:1674 >> +sub parseTypeOrExtendedType > > What is this parsing in the IDL grammer (http://www.w3.org/TR/WebIDL/#idl-grammar)? We'd like to make the parser as much as conformant with the IDL grammer. This is an excellent point. There is no concept of an "extended type" in the IDL grammar. This patch seems to associate the ExtendedAttributeList which a typedef may contain with the type that follows it. However, throughout the grammar, the ExtendedAttributeList is associated with a different production: Argument, Definitions, etc. The patch should be restructured to follow the grammar. Then it will make more sense, and confusion I had earlier about ignoring incoming ExtendedAttributeLists in certain circumstances will be cleared up. Fundamentally, what this patch should be doing is (a) doing the missing lookup in the typedef dictionary when parsing a type, and (b) merging the ExtendedAttributeLists that apply to Argument and those that were specified in the Typedef. Here are the key rules in http://dev.w3.org/2006/webapi/WebIDL/ : "No extended attributes defined in this specification are applicable to typedefs themselves." This means that in rule [1] Definitions, the ExtendedAttributeList must be empty if parsing a typedef. "Any extended attributes that apply to a construct due to their appearing on a referenced typedef are considered to appear after those that are specified explicitly on the construct..." This clearly means that it's legal to specify an ExtendedAttributeList for an Argument even if the Argument's type is a typedef, and that the extended attribute lists are concatenated. Later text states "none of the extended attributes defined in this specification that can validly be used in a typedef are sensitive to order." "A typedef must not be used as the type of a construct if an extended attribute that appears after the typedef keyword would be disallowed from appearing directly on that construct." You've pointed out this rule earlier. This means that the validation of the extended attribute list has to occur when the entire production (e.g. Argument) has been parsed, and the whole extended attribute list and type have been assembled. In the grammar, typedef lookup is handled in the NonAnyType production. > Source/WebCore/bindings/scripts/IDLParser.pm:1788 > + $self->assertNoExtendedAttributesInTypedef($next->value(), __LINE__); This assertion is incorrect. It's legal for an argument to have an extended attribute list and also, if the argument refers to a typedef, for the typedef to have had an ExtendedAttributeList. >> Source/WebCore/bindings/scripts/test/TestCallback.idl:32 >> +typedef Class2 TYPEDEF_CLASS2; > > Would you add new tests for 'typedef' instead of replacing existing tests? Replacing existing tests makes the intention of the existing tests unclear. Agree with this. Created attachment 188111 [details]
Patch
Attachment 188111 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/IDLParser.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefsPrivate.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefs.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefsInternal.h', u'Source/WebCore/bindings/scripts/test/TestTypedefs.idl', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.h']" exit_code: 1
Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefsInternal.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.cpp:32: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:150: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:152: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:153: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:154: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:159: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:161: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:162: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:163: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:164: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.h:59: More than one command on the same line in if [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp:46: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 14 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for the review.
I think I addressed all the comments (if I not missed something). Also I apply the typedefs post factum, according to the following WebIDL spec:
"Within an IDL fragment, a reference to a definition need not appear after the declaration of the referenced definition. References can also be made across IDL fragments."
"Therefore, the following IDL fragment is valid:
interface B : A {
void f(ArrayOfLongs x);
};
interface A {
};
typedef long[] ArrayOfLongs;
"
The only weak place is a regex replace to address union types, sequence<Type> and etc. A more honest approach, I think, would be to introduce new structs like Type, SingleType, UnionType, NonAnyType and etc. and return those from the parseType() and other subroutines. This seemed to me an overkill, provided that we do not support union types anyway.
Comment on attachment 188111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188111&action=review Thanks for the patch! > Source/WebCore/bindings/scripts/IDLParser.pm:361 > + if (exists $typedefs{$constant->type()}) { Nit: type() => type The same comments for other parts. > Source/WebCore/bindings/scripts/IDLParser.pm:363 > + $self->assertNoExtendedAttributesInTypedef($constant->type(), __LINE__); Is this speced? > Source/WebCore/bindings/scripts/IDLParser.pm:368 > + $self->applyTypedefsForSignature($attribute->signature()); Nit: signature() => signature > Source/WebCore/bindings/scripts/IDLParser.pm:392 > + my $type = $signature->type(); > + $type =~ s/[\?\[\]]+$//g; > + my $typeSuffix = $signature->type(); > + $typeSuffix =~ s/^[^\?\[\]]+//g; How about simply replacing a typedefed-type with a real type during parsing, instead of doing the replacement at the end of the parsing? By doing this, you don't need tricky regular expressions to replace types. Simply, every time you parse a type, you can check if the type is in the typedef structure. If it is found, you can just replace it at the point. As far as I read the spec, we can assume that typedef should appear before being used. So you don't need to apply the replacement at the end of parsing. http://www.w3.org/TR/WebIDL/#idl-typedefs Comment on attachment 188111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188111&action=review > Source/WebCore/bindings/scripts/test/TestTypedefs.idl:70 > +typedef float DOUBLE; > +typedef unsigned long long ULONGLONG; > +typedef [Clamp] unsigned long long ULONGLONG_CLAMP; > +typedef SerializedScriptValue SCRIPT_VALUE; > +typedef long[] ARRAY_OF_LONGS; > +typedef SVGPoint SVGPOINT; > +typedef DOMString STRING; > +typedef [Optional] DOMString OPTIONAL_STRING; > +typedef DOMString[] ARRAY_OF_STRINGS; > +typedef [Callback] TestCallback TEST_CALLBACK; > +typedef [TransferList=txx] SerializedScriptValue TRANSFER_TXX_SCRIPT_VALUE; You need to write them at the head of the file. (As far as I read the spec, I think that typedef should appear before being used.) Comment on attachment 188111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188111&action=review Thanks for persisting with this patch. It looks good to me at this point. Deferring to haraken for the remainder of the review since all of my concerns have been addressed. Happy to review further revisions though. > Source/WebCore/bindings/scripts/IDLParser.pm:358 > + foreach my $definition (@$definitions) { Is this descent into these data structures sufficient to find all the typedefs? What about for example raised exceptions? Can those refer to typedefs? >> Source/WebCore/bindings/scripts/IDLParser.pm:392 >> + $typeSuffix =~ s/^[^\?\[\]]+//g; > > How about simply replacing a typedefed-type with a real type during parsing, instead of doing the replacement at the end of the parsing? By doing this, you don't need tricky regular expressions to replace types. Simply, every time you parse a type, you can check if the type is in the typedef structure. If it is found, you can just replace it at the point. > > As far as I read the spec, we can assume that typedef should appear before being used. So you don't need to apply the replacement at the end of parsing. > http://www.w3.org/TR/WebIDL/#idl-typedefs Unfortunately as Andrey pointed out, a typedef can follow its first use in the file. http://www.w3.org/TR/WebIDL/#idl-names "Within an IDL fragment, ..." I agree this is pretty awful and wonder whether it's worth raising on public-script-coord. I wonder whether it's worth diverging from the Web IDL spec, and requiring that a typedef precede its first use, if it makes the implementation simpler and more correct. See comment above about whether the replacement of typedefs is fully correct. (In reply to comment #17) > Unfortunately as Andrey pointed out, a typedef can follow its first use in the file. http://www.w3.org/TR/WebIDL/#idl-names "Within an IDL fragment, ..." I agree this is pretty awful and wonder whether it's worth raising on public-script-coord. Thanks for clarification. Then the current patch looks OK to me, with a couple of nits I pointed out. I think it would be worth raising the issue to public discussion, but I don't want to block your work by that. So I'm happy to go with the current patch for now. > > Source/WebCore/bindings/scripts/IDLParser.pm:358 > > + foreach my $definition (@$definitions) { > > Is this descent into these data structures sufficient to find all the typedefs? What about for example raised exceptions? Can those refer to typedefs? (If we could mechanically replace a type with a typedef-ed type every time we find a type, this kind of concern would go away... I guess the current Web IDL spec is not good.) Comment on attachment 188111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188111&action=review >> Source/WebCore/bindings/scripts/IDLParser.pm:358 >> + foreach my $definition (@$definitions) { > > Is this descent into these data structures sufficient to find all the typedefs? What about for example raised exceptions? Can those refer to typedefs? We represent an exception with a domInterface struct (see parseException). As such they will be iterated below as well. I added test cases for those. >> Source/WebCore/bindings/scripts/IDLParser.pm:361 >> + if (exists $typedefs{$constant->type()}) { > > Nit: type() => type > > The same comments for other parts. done >> Source/WebCore/bindings/scripts/IDLParser.pm:363 >> + $self->assertNoExtendedAttributesInTypedef($constant->type(), __LINE__); > > Is this speced? WebIDL: "The type of a constant (matching ConstType) must not be any type other than a primitive type or a nullable primitive type. If an identifier is used, it must reference a typedef whose type is a primitive type or a nullable primitive type." So, I just disallow extended attributes here. The other checks, if we care, should already be there in some place. >> Source/WebCore/bindings/scripts/IDLParser.pm:368 >> + $self->applyTypedefsForSignature($attribute->signature()); > > Nit: signature() => signature done. >>> Source/WebCore/bindings/scripts/IDLParser.pm:392 >>> + $typeSuffix =~ s/^[^\?\[\]]+//g; >> >> How about simply replacing a typedefed-type with a real type during parsing, instead of doing the replacement at the end of the parsing? By doing this, you don't need tricky regular expressions to replace types. Simply, every time you parse a type, you can check if the type is in the typedef structure. If it is found, you can just replace it at the point. >> >> As far as I read the spec, we can assume that typedef should appear before being used. So you don't need to apply the replacement at the end of parsing. >> http://www.w3.org/TR/WebIDL/#idl-typedefs > > Unfortunately as Andrey pointed out, a typedef can follow its first use in the file. http://www.w3.org/TR/WebIDL/#idl-names "Within an IDL fragment, ..." I agree this is pretty awful and wonder whether it's worth raising on public-script-coord. > > I wonder whether it's worth diverging from the Web IDL spec, and requiring that a typedef precede its first use, if it makes the implementation simpler and more correct. See comment above about whether the replacement of typedefs is fully correct. @haraken: >Simply, every time you parse a type, you can check if the type is in the typedef structure. If it is found, you can just replace it at the point. Even if we require typedefs to precede its first use, what to do with it's extended attributes? Those may be applied to a const or an attribute or a function/constructor signature - we don't have an access to these at the parseNonAnyType() function. Created attachment 188276 [details]
Patch
Attachment 188276 [details] did not pass style-queue:
Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefsInternal.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.cpp:32: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:190: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:192: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:193: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:194: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:197: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:199: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:201: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:202: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:203: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:204: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:206: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:208: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:209: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:210: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:213: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:215: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:217: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:218: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:219: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:222: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:224: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:226: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:227: Weird number of spaces at line-start. Are you usinFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/IDLParser.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefsPrivate.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefs.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefsInternal.h', u'Source/WebCore/bindings/scripts/test/TestTypedefs.idl', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.h']" exit_code: 1
g a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:228: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:229: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:231: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.h:59: More than one command on the same line in if [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp:46: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 34 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 188276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188276&action=review > Source/WebCore/bindings/scripts/IDLParser.pm:392 > + $type =~ s/[\?\[\]]+$//g; > + my $typeSuffix = $signature->type; > + $typeSuffix =~ s/^[^\?\[\]]+//g; Do you need this? It seems that you are assuming something like: typedef Foo Bar; attribute Bar[] xxx; (In reply to comment #22) > (From update of attachment 188276 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188276&action=review > > > Source/WebCore/bindings/scripts/IDLParser.pm:392 > > + $type =~ s/[\?\[\]]+$//g; > > + my $typeSuffix = $signature->type; > > + $typeSuffix =~ s/^[^\?\[\]]+//g; > > Do you need this? > > It seems that you are assuming something like: > > typedef Foo Bar; > attribute Bar[] xxx; Exactly. (In reply to comment #23) > > typedef Foo Bar; > > attribute Bar[] xxx; > > Exactly. From the spec, it is not clear to me if we should support it or not. Would you elaborate on that? If we should support it, please add a test case. (In reply to comment #24) > (In reply to comment #23) > > > typedef Foo Bar; > > > attribute Bar[] xxx; > > > > Exactly. > > From the spec, it is not clear to me if we should support it or not. Would you elaborate on that? > > If we should support it, please add a test case. [24] Typedef → "typedef" ExtendedAttributeList Type identifier ";" [62] NonAnyType → PrimitiveType TypeSuffix | "DOMString" TypeSuffix | identifier TypeSuffix | "sequence" "<" Type ">" Null | "object" TypeSuffix | "Date" TypeSuffix So, if a NonAnyType == "identifier TypeSuffix", the identifier may point to a typedef, resolving to "ExtendedAttributeList Type TypeSuffix". At least this is how I understand the spec. I'll add a test. Created attachment 188280 [details]
with a test case for STRING[]
(In reply to comment #25) > [24] Typedef → "typedef" ExtendedAttributeList Type identifier ";" > > [62] NonAnyType → PrimitiveType TypeSuffix > | "DOMString" TypeSuffix > | identifier TypeSuffix > | "sequence" "<" Type ">" Null > | "object" TypeSuffix > | "Date" TypeSuffix > > So, if a NonAnyType == "identifier TypeSuffix", the identifier may point to a typedef, resolving to "ExtendedAttributeList Type TypeSuffix". At least this is how I understand the spec. > > I'll add a test. Thanks, you're right. I think the current Web IDL spec is not good. I'm happy to go with the current patch for now, but would you raise the issue to the community? Comment on attachment 188280 [details]
with a test case for STRING[]
LGTM. Thanks!
Attachment 188280 [details] did not pass style-queue:
Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefsInternal.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.cpp:32: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:190: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:192: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:193: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:194: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:197: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:199: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:201: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:202: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:203: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:204: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:206: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:208: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:209: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:210: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:213: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:215: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:217: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:218: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:219: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:222: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:224: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:226: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:227: Weird number of spaces at line-start. Are you usinFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/IDLParser.pm', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/CPP/WebDOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefsPrivate.h', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefs.h', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefs.mm', u'Source/WebCore/bindings/scripts/test/ObjC/DOMTestTypedefsInternal.h', u'Source/WebCore/bindings/scripts/test/TestTypedefs.idl', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.h']" exit_code: 1
g a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:228: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:229: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:231: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestTypedefs.cpp:236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.h:59: More than one command on the same line in if [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp:46: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 34 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 188280 [details] with a test case for STRING[] Clearing flags on attachment: 188280 Committed r142865: <http://trac.webkit.org/changeset/142865> All reviewed patches have been landed. Closing bug. binding tests started failing after this patch: http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/5943/steps/bindings-generation-tests/logs/stdio --- WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp 2013-02-14 04:38:13.000000000 -0800 +++ /var/folders/40/g3vmqrl1701740n0crx6dcjw0000gn/T/tmpCKB29V/V8TestTypedefs.cpp 2013-02-14 05:16:39.000000000 -0800 @@ -286,7 +286,7 @@ TestTypedefs* imp = V8TestTypedefs::toNative(args.Holder()); unsigned long long arg1 = 0; V8TRYCATCH(double, arg1NativeValue, args[0]->NumberValue()); - if (!isnan(arg1NativeValue)) + if (!std::isnan(arg1NativeValue)) arg1 = clampTo<unsigned long long>(arg1NativeValue); if (args.Length() <= 1) { imp->funcWithClamp(arg1); @@ -294,7 +294,7 @@ } unsigned long long arg2 = 0; V8TRYCATCH(double, arg2NativeValue, args[1]->NumberValue()); - if (!isnan(arg2NativeValue)) + if (!std::isnan(arg2NativeValue)) arg2 = clampTo<unsigned long long>(arg2NativeValue); imp->funcWithClamp(arg1, arg2); return v8Undefined(); Rebaselined run-bindings-tests in r142951. |