The default bug view has changed. See this FAQ.

Codegen: Remove generateNativeAccessors branching.

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
We are now pretty comitted to using native accessors. We should be able to rip out the code that generates PropertyOps.

Updated

5 years ago
Blocks: 580070
(Assignee)

Comment 1

5 years ago
Created attachment 644085 [details] [diff] [review]
Patch

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.
(Assignee)

Updated

5 years ago
Attachment #644085 - Attachment is patch: true
Attachment #644085 - Flags: review?(peterv)
Comment on attachment 644085 [details] [diff] [review]
Patch

>+++ b/dom/bindings/Codegen.py
>@@ -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 Codegen.py 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 GlobalGen.py in main()
* The useJSOPAccessors command-line option handling in the same function
* Same two things in BindingGen.py
* The various mentions of ACCESSOR_OPT in the Makefile.in
(Assignee)

Comment 3

5 years ago
Created attachment 644180 [details] [diff] [review]
Best to remove it all the way.
Attachment #644085 - Attachment is obsolete: true
Attachment #644085 - Flags: review?(peterv)
Attachment #644180 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #644180 - Flags: review?
(Assignee)

Comment 4

5 years ago
Created attachment 644181 [details]
Codegen.py before centralization

Codegen.py 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.
(Assignee)

Comment 6

5 years ago
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/Codegen.py
@@ +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?
(Assignee)

Comment 9

5 years ago
Created attachment 644545 [details] [diff] [review]
Remove GetterSetterCall as well
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/Codegen.py
@@ +3171,5 @@
>                  argv +
>                  ("if (!specialized_set_%s(cx, obj, self, argv)) {\n"
>                   "  return false;\n"
>                   "}\n" % self.attr.identifier.name) +
>                  retval +

Just replace the variables with their values here.
Attachment #644545 - Flags: review?(peterv) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/532d31e06fb8
https://hg.mozilla.org/mozilla-central/rev/532d31e06fb8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.