Last Comment Bug 775289 - Codegen: Remove generateNativeAccessors branching.
: Codegen: Remove generateNativeAccessors branching.
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Eric Faust [:efaust]
: Andrew Overholt [:overholt]
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
Reported: 2012-07-18 14:05 PDT by Eric Faust [:efaust]
Modified: 2012-08-08 09:28 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (7.18 KB, patch)
2012-07-19 17:33 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Best to remove it all the way. (12.28 KB, patch)
2012-07-19 22:42 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review before centralization (185.70 KB, text/plain)
2012-07-19 22:45 PDT, Eric Faust [:efaust]
no flags Details
Remove GetterSetterCall as well (13.72 KB, patch)
2012-07-20 18:11 PDT, Eric Faust [:efaust]
peterv: review+
Details | Diff | Splinter Review

Description Eric Faust [:efaust] 2012-07-18 14:05:56 PDT
We are now pretty comitted to using native accessors. We should be able to rip out the code that generates PropertyOps.
Comment 1 Eric Faust [:efaust] 2012-07-19 17:33:36 PDT
Created attachment 644085 [details] [diff] [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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 22:04:37 PDT
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
Comment 3 Eric Faust [:efaust] 2012-07-19 22:42:46 PDT
Created attachment 644180 [details] [diff] [review]
Best to remove it all the way.
Comment 4 Eric Faust [:efaust] 2012-07-19 22:45:16 PDT
Created attachment 644181 [details] before centralization in light of the patch above.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 23:01:42 PDT
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.
Comment 6 Eric Faust [:efaust] 2012-07-19 23:15:18 PDT
Right, the only question was whether to cut CGGetterSetterCall. Since it holds basically no purpose, I'll just strike it.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 23:30:27 PDT
Well, it commons up some details of initing CGPerSignatureCall.  But yeah, nixing it is fine.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-07-20 02:34:46 PDT
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?
Comment 9 Eric Faust [:efaust] 2012-07-20 18:11:05 PDT
Created attachment 644545 [details] [diff] [review]
Remove GetterSetterCall as well
Comment 10 Peter Van der Beken [:peterv] 2012-08-02 03:04:43 PDT
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.
Comment 12 Ed Morley [:emorley] 2012-08-08 09:28:10 PDT

Note You need to log in before you can comment on or make changes to this bug.