Comment on attachment 787963 [details] [diff] [review] Convert union return values Review of attachment 787963 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +6199,5 @@ > " return true;\n" > " }") > conversionsToJS.extend( > map(self.getConversionToJS, > + zip(self.templateVars, self.type.flatMemberTypes))) Pre-existing: this could probably be a little more efficient with extend(self.getConversionToJS(arg) for arg in zip(self.templateVars, self.type.flatMemberTypes)) @@ +6206,1 @@ > switch (mType) { Maybe a switch class would be useful too @@ +6223,5 @@ > + > + members = [ClassMember("mType", > + "Type"), > + ClassMember("mValue", > + "Value")] This can be two lines @@ +6225,5 @@ > + "Type"), > + ClassMember("mValue", > + "Value")] > + ctor = ClassConstructor([], bodyInHeader=True, visibility="public", > + body="mType = eUninitialized;") Probably want explicit=True? Also, pass body="eUninitialized" to the ClassMember, then you can drop the assignment here.

Comment on attachment 788258 [details] [diff] [review] Convert union return values >+ def toJSValMethod(self): >+ switch (mType) { Bonus points for using CGSwitch here, but followup ok. If you do it here, I'd like to see the updated patch. >+ return ClassMethod("ToJSVal", "bool", [ This should have const=True to preserve the old behavior. >+ def getStruct(self): >+ body="""mType = e%s; >+return mValue.m%s.SetValue();""" % (vars["name"], vars["name"]) This should probably use ${name} inside the template and string.Template stuff, to avoid repeating the vars["name"] thing. It might also be cleaner to use strings without """ and with explicit \n like so: body=string.Template("mType = e${name};\n" "return mValue.m${name}.SetValue();").substitute(vars) or so. >+ unionValues.append("UnionMember<%s > m%s;" % (vars["structType"], >+ vars["name"])) Again, better to use ${structType} and ${name} in the string and string.Template().substitute. >+ callDestructors += """ Again, please use string.Template. And a followup to use CGSwitch? >+ def __init__(self, name, entries, values=None, visibility="public"): Take out the values bits: there can't be any for a union. >+ entries = [] >+ for i in range(0, len(self.entries)): >+ if not self.values or i >= len(self.values): >+ entry = '%s' % self.entries[i] And this can all go away... >+ name = '' if not self.name else ' ' + self.name Can you ever have a union with no name? Can happen for an enum, but I think for a union that's not possible. >+ return 'union%s\n{\n %s\n};\n' % (name, '\n '.join(entries)) So here just pass self.entries to join(), or perhaps better yet: entries = entry + ";" for entry in self.entries and take out your ';' up above where you set up the CGUnion. > class CGClass(CGThing): >+ return """%s(const %s&) MOZ_DELETE; >+void operator=(const %s) MOZ_DELETE;\n""" % (name, name, name) Again, I'd prefer using "" and explicit \n. r=me with the above fixed

Comment on attachment 788478 [details] [diff] [review] Use CGSwitch r=me

Comment on attachment 788479 [details] [diff] [review] Make CGUnionStruct use CGClass Please document why the Destroy* methods have bodyInHeader=not self.isReturnValue. >+ body = string.Template("MOZ_ASSERT(Is${name}(), \"Wrong type!\");\n" Could also do: body = string.Template('MOZ_ASSERT(Is${name}(), "Wrong type!");\n' which is not as backslashy. This comes up a few times in this patch. >+ return CGClass(self.type.__str__() + ( "ReturnValue" if self.isReturnValue else ""), Drop the extra space after '(' there. r=me with the nits

https://hg.mozilla.org/mozilla-central/rev/cb152e57f23b https://hg.mozilla.org/mozilla-central/rev/83a7277e67bc https://hg.mozilla.org/mozilla-central/rev/260786f8fef0

Comment on attachment 793144 [details] [diff] [review] Make CGUnionConversionStruct use CGClass >+ "}") % name + tryNextCode I think making that be: "}" % name) + tryNextCode would be clearer. >+ return CGClass(str(self.type) + "Argument", structName + "Argument" perhaps? r=me