Last Comment Bug 767352 - Switch WebIDL parser to create one object per method override
: Switch WebIDL parser to create one object per method override
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 742152
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-22 06:34 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-07-07 12:03 PDT (History)
2 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Keep track of a method's overloads in a list of tuples. (11.29 KB, patch)
2012-06-25 22:04 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Now with object (13.17 KB, patch)
2012-06-29 10:47 PDT, Boris Zbarsky [:bz]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-06-22 06:34:35 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2012-06-25 22:04:35 PDT
Created attachment 636596 [details] [diff] [review]
Keep track of a method's overloads in a list of tuples.
Comment 2 Justin Lebar (not reading bugmail) 2012-06-29 08:27:34 PDT
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
Comment 3 Boris Zbarsky [:bz] 2012-06-29 08:55:54 PDT
We can't use namedtuple: needs 2.6.

I did consider a class.  Maybe I should do that...
Comment 4 Boris Zbarsky [:bz] 2012-06-29 10:47:36 PDT
Created attachment 637940 [details] [diff] [review]
Now with object
Comment 5 Justin Lebar (not reading bugmail) 2012-07-02 03:37:42 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2012-07-06 08:31:21 PDT
> 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.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-07 12:03:24 PDT
https://hg.mozilla.org/mozilla-central/rev/5ceed3703df9

Note You need to log in before you can comment on or make changes to this bug.