Closed Bug 775289 Opened 7 years ago Closed 7 years ago

Codegen: Remove generateNativeAccessors branching.


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

Not set





(Reporter: efaust, Assigned: efaust)


(Blocks 1 open bug)



(2 files, 2 obsolete files)

We are now pretty comitted to using native accessors. We should be able to rip out the code that generates PropertyOps.
Attached patch Patch (obsolete) — Splinter Review
Applies on top of patches in bug 773548.

Can we do better with CG{Getter,Setter,GetterSetter}Call? It feels like they should be able to go away, but I didn't want to remove the safety from getter != setter.
Attachment #644085 - Attachment is patch: true
Attachment #644085 - Flags: review?(peterv)
Comment on attachment 644085 [details] [diff] [review]

>+++ b/dom/bindings/
>@@ -949,20 +949,18 @@ class AttrDefiner(PropertyDefiner):
>-            elif attr.readonly:
>+            if attr.readonly:
>                 return "JSPROP_READONLY | " + flags

Shouldn't the attr.readonly branch go away altogether?  Note that it was not used if generateNativeAccessors, because JSPROP_READONLY is nonsensical for an accessor property, iirc.

The rest looks fine.  I'm not sure yet what to do with CG*Call; want to attach your as it looks after these changes?

In addition to the changes in this diff, you can also remove the following:

* The setting of Codegen.generateNativeAccessors in in main()
* The useJSOPAccessors command-line option handling in the same function
* Same two things in
* The various mentions of ACCESSOR_OPT in the
Attached patch Best to remove it all the way. (obsolete) — Splinter Review
Attachment #644085 - Attachment is obsolete: true
Attachment #644085 - Flags: review?(peterv)
Attachment #644180 - Flags: review?
Attachment #644180 - Flags: review? in light of the patch above.
Attachment #644181 - Attachment mime type: text/x-python-script → text/plain
So I think you can nix CGGetterSetterCall and just have getter and setter directly inherit from CGPerSignatureCall if you wanted to.  Or you could just leave it as-is.

You can't really common together CGGetterCall and CGSetterCall easily because they actually do different things in various ways.
Right, the only question was whether to cut CGGetterSetterCall. Since it holds basically no purpose, I'll just strike it.
Well, it commons up some details of initing CGPerSignatureCall.  But yeah, nixing it is fine.
Comment on attachment 644180 [details] [diff] [review]
Best to remove it all the way.

Review of attachment 644180 [details] [diff] [review]:

::: dom/bindings/
@@ +3173,3 @@
>      def generate_code(self):
> +        argv = ("JS::Value* argv = JS_ARGV(cx, vp);\n"
> +                "jsval undef = JS::UndefinedValue();\n"

Make this JS::Value while you're here?
Attachment #644180 - Attachment is obsolete: true
Attachment #644545 - Flags: review?(peterv)
Comment on attachment 644545 [details] [diff] [review]
Remove GetterSetterCall as well

Review of attachment 644545 [details] [diff] [review]:

::: dom/bindings/
@@ +3171,5 @@
>                  argv +
>                  ("if (!specialized_set_%s(cx, obj, self, argv)) {\n"
>                   "  return false;\n"
>                   "}\n" % +
>                  retval +

Just replace the variables with their values here.
Attachment #644545 - Flags: review?(peterv) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.