Closed Bug 883493 Opened 7 years ago Closed 7 years ago

Switch CGUnionStruct and CGUnionReturnValueStruct to use CGClass

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
Duplicate of this bug: 865098
Attached patch Convert union return values (obsolete) — Splinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #787963 - Flags: review?
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.
Attachment #787963 - Flags: review? → review?(bzbarsky)
Attachment #787963 - Attachment is obsolete: true
Attachment #787963 - Flags: review?(bzbarsky)
Attachment #788258 - Flags: review?(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
Attachment #788258 - Flags: review?(bzbarsky) → review+
Attached patch Use CGSwitchSplinter Review
Attachment #788478 - Flags: review?(bzbarsky)
Attachment #788479 - Flags: review?(bzbarsky)
Comment on attachment 788478 [details] [diff] [review]
Use CGSwitch

r=me
Attachment #788478 - Flags: review?(bzbarsky) → review+
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
Attachment #788479 - Flags: review?(bzbarsky) → review+
Depends on: 905542
Attachment #793136 - Flags: review?(bzbarsky)
Attachment #793136 - Attachment is obsolete: true
Attachment #793136 - Flags: review?(bzbarsky)
Attachment #793144 - Flags: review?(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
Attachment #793144 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/2cf6fdf25625
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.