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 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...

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.

> 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.