Clean up forward class declarations

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
This kind of annoys me, and now that I've looked at this code a bit I think it shouldn't be too hard to fix.

right now we do:
namespace mozilla {
namespace dom {

class RTCPeerConnection;

} // namespace dom
} // namespace mozilla

namespace mozilla {
namespace dom {

class RTCPeerConnection;

} // namespace dom
} // namespace mozilla


But this would be a little more space efficiently done more like:

namespace mozilla {
namespace dom {

class RTCPeerConnection;
class RTCPeerConnection;

} // namespace dom
} // namespace mozilla
(Assignee)

Comment 1

6 years ago
Oh, and also we could delete duplicates. ;)
Yes, please!  And sort alphabetically?
(Assignee)

Comment 3

6 years ago
I'll work on this after I finish bug 863880.  I'll undo the refactorings in that bug, if I'm going to end up rewriting the mechanism anyway for this bug.

So we have
 -- nest together things declared in the mozilla::dom::namespace
 -- eliminate duplicates
 -- sort alphabetically

Let me know if you think of anything else.  I'll also take a look at the headers we generate and see if anything else springs out at me after the above are fixed up.
Assignee: nobody → continuation
Summary: Group together forward declared classes from the mozilla::dom:: namespace → Clean up forward class declarations
(Assignee)

Comment 4

6 years ago
Created attachment 740107 [details] [diff] [review]
clean up forward declarations in codegen

This was bothering me, so I ended up just fixing it.  I tried to come up with reasonable heuristics for adding vertical whitespace.  This takes the number of lines in TestCodeGenBinding.h used for forward declarations from 322 down to 31.
Attachment #740107 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

6 years ago
Created attachment 740108 [details]
contrived and actual examples of usage, output
(Assignee)

Comment 6

6 years ago
Comment on attachment 740107 [details] [diff] [review]
clean up forward declarations in codegen

This doesn't actually alphabetize.
Attachment #740107 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

6 years ago
Created attachment 740110 [details]
contrived and actual examples of usage, output
Attachment #740108 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 740111 [details] [diff] [review]
clean up forward declarations in codegen
Attachment #740107 - Attachment is obsolete: true
Attachment #740111 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Blocks: 863880
Comment on attachment 740111 [details] [diff] [review]
clean up forward declarations in codegen

>+    def listAdd(self, namespaces, name, isStruct=False):

I think we should name this _listAdd, since it's not really public API.

>+    def build(self, atTopLevel=True):

Is it worth having a public build() with no arguments and a _build with the optional arg?

>+                         for (cname, isStruct) in sorted(list(self.decls))]))

I think just sorted(self.decls) should work fine.

>+        for namespace, child in sorted(self.children.iteritems()):

What's this sorting the items on?  I see nothing that defines how tuples actually sort...

I think what should happen here is a sort on the first element of the tuple, using an appropriate key= lambda.

>+        if not atTopLevel and (len(decls) > 1 or len(self.decls) > 1):

Or maybe len(decls) + len(self.decls) > 1?  Either way, I guess.

r=me with the above dealt with.
Attachment #740111 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

6 years ago
> What's this sorting the items on?  I see nothing that defines how tuples actually sort...

This page says that tuples are compared lexicographically: http://wiki.python.org/moin/HowTo/Sorting/
Ah, excellent.
(Assignee)

Comment 13

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #9)
> I think we should name this _listAdd, since it's not really public API.
Done.

> Is it worth having a public build() with no arguments and a _build with the
> optional arg?
I did that, except that I made the argument to _build non-optional because we pass it everywhere.

> I think just sorted(self.decls) should work fine.
Done.

> Or maybe len(decls) + len(self.decls) > 1?  Either way, I guess.
Done.
https://hg.mozilla.org/mozilla-central/rev/ceac3c6904d6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.