883493 - Switch CGUnionStruct and CGUnionReturnValueStruct to use CGClass

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Convert union return values

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 4

[David Zbarsky (:dzbarsky)]

Attachment: Convert union return values

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb152e57f23b

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Use CGSwitch

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Make CGUnionStruct use CGClass

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 788478 [details] [diff] [review]
Use CGSwitch

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Carsten Book [:Tomcat]]

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 12

[David Zbarsky (:dzbarsky)]

Attachment: Make CGUnionConversionStruct use CGClass

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: Make CGUnionConversionStruct use CGClass

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf6fdf25625

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2cf6fdf25625