Switch WebIDL parser to create one object per method override

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: bz)

Tracking

Trunk
mozilla16
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Right now we have

  IDLMethod:
    self._returnValue # list of return values, one for each overload
    self._arguments   # list of list of arguments, one for each overload

And bug 742152 adds

    self._location    # list of locations, one for each overload

Maintaining parallel lists is silly and hard.  We should instead have self._overloads with one (return value, args, location) object per overload.
Created attachment 636596 [details] [diff] [review]
Keep track of a method's overloads in a list of tuples.
Attachment #636596 - Flags: review?(justin.lebar+bug)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
(Reporter)

Comment 2

5 years ago
Comment on attachment 636596 [details] [diff] [review]
Keep track of a method's overloads in a list of tuples.

Do you think this would be better if you used a named tuple (or a proper class) for overloads?  There's a lot of packing and unpacking tuples here, and if we wanted to add another field on an overload, we'd be in trouble.

http://docs.python.org/library/collections.html#collections.namedtuple
We can't use namedtuple: needs 2.6.

I did consider a class.  Maybe I should do that...
Created attachment 637940 [details] [diff] [review]
Now with object
Attachment #637940 - Flags: review?(justin.lebar+bug)
Attachment #636596 - Attachment is obsolete: true
Attachment #636596 - Flags: review?(justin.lebar+bug)
(Reporter)

Comment 5

5 years ago
I should bookmark the Python 2.5 docs so I don't keep suggesting features that are too new for us.  :-/

>     def finish(self, scope):
>-        for index, returnType in enumerate(self._returnType):
>-            if returnType.isComplete():
>-                continue
>-
>-            type = returnType.complete(scope)
>-
>-            assert not isinstance(type, IDLUnresolvedType)
>-            assert not isinstance(type.name, IDLUnresolvedIdentifier)
>-            self._returnType[index] = type
>-
>-        for arguments in self._arguments:
>+        for index, overload in enumerate(self._overloads):
>+            returnType = overload.returnType
>+            arguments = overload.arguments
>+            location = overload.location
>+
>             for argument in arguments:
>                 if argument.type.isComplete():
>                     continue
> 
>                 type = argument.type.complete(scope)
> 
>                 assert not isinstance(type, IDLUnresolvedType)
>                 assert not isinstance(type.name, IDLUnresolvedIdentifier)
>                 argument.type = type
> 
>+            if returnType.isComplete():
>+                continue
>+
>+            type = returnType.complete(scope)
>+
>+            assert not isinstance(type, IDLUnresolvedType)
>+            assert not isinstance(type.name, IDLUnresolvedIdentifier)
>+            self._overloads[index] = IDLMethodOverload(type, arguments,
>+                                                       location)

Is there a reason not to modify overload.returnType?  Then you wouldn't have to
enumerate and so on.

>     def validate(self):
>         # Make sure our overloads are properly distinguishable and don't have
>         # different argument types before the distinguishing args.
>         for argCount in self.allowedArgCounts:
>-            possibleSignatures = self.signaturesForArgCount(argCount)
>-            if len(possibleSignatures) == 1:
>+            possibleOverloads = self.overloadsForArgCount(argCount)
>+            if len(possibleOverloads) == 1:
>                 continue
>             distinguishingIndex = self.distinguishingIndexForArgCount(argCount)
>-            arglists = [ s[1] for s in possibleSignatures ]
>+            arglists = [ overload.arguments for overload in possibleOverloads ]
>             for idx in range(distinguishingIndex):
>-                firstSigType = arglists[0][idx].type
>-                for (otherArgList, location) in zip(arglists[1:],
>-                                                    self._location[1:]):
>-                    if otherArgList[idx].type != firstSigType:
>+                firstSigType = possibleOverloads[0].arguments[idx].type
>+                for overload in possibleOverloads[1:]:
>+                    if overload.arguments[idx].type != firstSigType:
>                         raise WebIDLError(
>                             "Signatures for method '%s' with %d arguments have "
>                             "different types of arguments at index %d, which "
>                             "is before distinguishing index %d" %
>                             (self.identifier.name, argCount, idx,
>                              distinguishingIndex),
>-                            [self.location, location])
>+                            [self.location, overload.location])

You don't use arglists anymore.

>+    def overloadsForArgCount(self, argc):
>+        # Preserves the locations

Nit: This comment doesn't make much sense anymore.

>+        return [overload for overload in self._overloads if
>+                len(overload.arguments) == argc or
>+                (len(overload.arguments) > argc and
>+                 overload.arguments[argc].optional)]
> 
>     def signaturesForArgCount(self, argc):
>-        return [(retval, args) for (retval, args) in self.signatures() if
>-                len(args) == argc or (len(args) > argc and args[argc].optional)]
>+        return [(overload.returnType, overload.arguments) for overload
>+                in self.overloadsForArgCount(argc)]
> 
>     def locationsForArgCount(self, argc):
>-        return [ self._location[i] for (i, args) in enumerate(self._arguments) if
>-                 len(args) == argc or
>-                 (len(args) > argc and args[argc].optional)]
>+        return [ overload.location for overload in self._overloads if
>+                 len(overload.arguments) == argc or
>+                 (len(overload.arguments) > argc and
>+                  overload.arguments[argc].optional)]

Nit: We should probably be consistent about space after list comprehension '[' at least within the same patch hunk.

It seems to me that the codegen should use only overloadsForArgCount, but we don't have to fix that here.
(Reporter)

Updated

5 years ago
Attachment #637940 - Flags: review?(justin.lebar+bug) → review+
> Is there a reason not to modify overload.returnType?

No.  When it was a tuple, I couldn't do that, but now of course I can.  Will fix.

> You don't use arglists anymore.
> Nit: This comment doesn't make much sense anymore.
> We should probably be consistent about space after list comprehension

Fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ceed3703df9
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/5ceed3703df9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.