Closed Bug 863964 Opened 7 years ago Closed 7 years ago

Clean up forward class declarations

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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
Oh, and also we could delete duplicates. ;)
Yes, please!  And sort alphabetically?
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
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)
Comment on attachment 740107 [details] [diff] [review]
clean up forward declarations in codegen

This doesn't actually alphabetize.
Attachment #740107 - Flags: review?(bzbarsky)
Attachment #740107 - Attachment is obsolete: true
Attachment #740111 - Flags: review?(bzbarsky)
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+
> 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.
(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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.