Closed
Bug 863964
Opened 11 years ago
Closed 11 years ago
Clean up forward class declarations
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
2.05 KB,
text/plain
|
Details | |
8.34 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Oh, and also we could delete duplicates. ;)
Comment 2•11 years ago
|
||
Yes, please! And sort alphabetically?
Assignee | ||
Comment 3•11 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•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 6•11 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•11 years ago
|
||
Attachment #740108 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #740107 -
Attachment is obsolete: true
Attachment #740111 -
Flags: review?(bzbarsky)
Comment 9•11 years ago
|
||
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•11 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/
Comment 11•11 years ago
|
||
Ah, excellent.
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceac3c6904d6
Assignee | ||
Comment 13•11 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.
Assignee | ||
Comment 14•11 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=7ddb2c175154
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ceac3c6904d6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•