bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Use CGClass for CGDictionary

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 750967 [details] [diff] [review]
Part a: CGClass fixes
Attachment #750967 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

5 years ago
Created attachment 750969 [details] [diff] [review]
Part b: Use CGClasses for CGDictionary
Attachment #750969 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 750970 [details] [diff] [review]
Diff for TestCodeGenBinding.cpp
Comment on attachment 750967 [details] [diff] [review]
Part a: CGClass fixes

>+        self.baseConstructors = baseConstructors or []

Why not just make the default value in the call signature [] instead of None?  And fix the comment about what it defaults to.

Speaking of which, are you willing to write a nice doc comment for CGClass.__init__ explaining what the various arguments mean?  For example what sorts of objects are expected in constructors, members, bases, etc?

>-        if len(items) > 0:
>+        if items:

Why this change?  items is guaranteed to be a list, right?  Seems to me like the length check is more explicit about what's going on.

>+                    # Member variables would only produce empty lines here.

Isn't it just the case that we don't want to include self.members in "order" when doing define()?  If we _do_ want to include them, worth documenting why.

>-        order = [(self.members, '\n'), (self.constructors, '\n'),
>+        order = [(self.members, ''), (self.constructors, ''),

Again, why the change?  Seems like having newlines there might not be a bad idea....
Attachment #750967 - Flags: review?(bzbarsky) → review-
> Isn't it just the case that we don't want to include self.members in "order" 

Ah, but static members we do want to include, ok.  So this part makes sense.
> Seems like having newlines there might not be a bad idea....

Or is there already a bunch of newline built into things somehow?  The diff on the generated code suggests yes, but I'm not sure where they're coming from.
Comment on attachment 750967 [details] [diff] [review]
Part a: CGClass fixes

OK, r+ with the newline stuff explained or fixed and the other nits addressed.
Attachment #750967 - Flags: review- → review+
Comment on attachment 750969 [details] [diff] [review]
Part b: Use CGClasses for CGDictionary

>+    def bases(self):
>+        return []

ClassBase("DictionaryBase"), please.  Otherwise the rooting stuff won't work right for worker dictionaries (hopefully by just not compiling).

>+        return ClassMethod("Init", "bool", [
>+            Argument('JSContext*', 'cx'),
>+            Argument('JS::Handle<JS::Value>', 'val'),
>+        ], body=body)

The indentation here is a bit weird.  I'd probably do it as:

        return ClassMethod("Init", "bool",
                           [ Argument('JSContext*', 'cx'),
                             Argument('JS::Handle<JS::Value>', 'val') ],
                           body=body)

and no matter what take out the trailing ',' in the args list.  Or is that trying to minimize future diffs if we change the arguments?

>+    def initFromJSONMethod(self):

Again, the indentation is a bit weird.  This probably apples to all the ClassMethods in here.

>+    def getStructs(self):
>+            members.extend([

No need for the square brackets inside extend().

This is awesome.  Way more maintainable now.  r=me
Attachment #750969 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Depends on: 874033
(Assignee)

Comment 9

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 750967 [details] [diff] [review]
> Part a: CGClass fixes
> 
> >+        self.baseConstructors = baseConstructors or []
> 
> Why not just make the default value in the call signature [] instead of
> None?  And fix the comment about what it defaults to.

http://effbot.org/zone/default-values.htm

> Speaking of which, are you willing to write a nice doc comment for
> CGClass.__init__ explaining what the various arguments mean?  For example
> what sorts of objects are expected in constructors, members, bases, etc?

Sure; filed bug 874033.

> >-        if len(items) > 0:
> >+        if items:
> 
> Why this change?  items is guaranteed to be a list, right?  Seems to me like
> the length check is more explicit about what's going on.

Reverted.

> >-        order = [(self.members, '\n'), (self.constructors, '\n'),
> >+        order = [(self.members, ''), (self.constructors, ''),
> 
> Again, why the change?  Seems like having newlines there might not be a bad
> idea....

Dealt with those.

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 750969 [details] [diff] [review]
> Part b: Use CGClasses for CGDictionary
> 
> >+    def bases(self):
> >+        return []
> 
> ClassBase("DictionaryBase"), please.  Otherwise the rooting stuff won't work
> right for worker dictionaries (hopefully by just not compiling).

I ended up just returning a string here.

> >+        return ClassMethod("Init", "bool", [
> >+            Argument('JSContext*', 'cx'),
> >+            Argument('JS::Handle<JS::Value>', 'val'),
> >+        ], body=body)
> 
> The indentation here is a bit weird.  I'd probably do it as:
> 
>         return ClassMethod("Init", "bool",
>                            [ Argument('JSContext*', 'cx'),
>                              Argument('JS::Handle<JS::Value>', 'val') ],
>                            body=body)
> 
> and no matter what take out the trailing ',' in the args list.  Or is that
> trying to minimize future diffs if we change the arguments?

I don't think that was an improvement, so I left this alone. Trailing comma is a habit; originally to minimize diffs, yes. Let me know if you feel strongly about this.

> >+    def initFromJSONMethod(self):
> 
> Again, the indentation is a bit weird.  This probably apples to all the
> ClassMethods in here.
> 
> >+    def getStructs(self):
> >+            members.extend([
> 
> No need for the square brackets inside extend().

Fixed.

https://hg.mozilla.org/mozilla-central/rev/60a7f4f3edd9
https://hg.mozilla.org/mozilla-central/rev/1a4f97b79578
Target Milestone: --- → mozilla24
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
> http://effbot.org/zone/default-values.htm

Not relevant is we don't modify self.baseConstructors, right?  Do we ever modify it?

> Let me know if you feel strongly about this.

It just means that whenever someone with an auto-indenting editor edits this it will get reindented, as will things like this:

   1.353 +        initializerStruct = CGClass(selfName + "Initializer",
   1.354 +            bases=[ClassBase(selfName)],
   1.355 +            constructors=[initializerCtor],
   1.356 +            isStruct=True)

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