Split up builtin/Stream.* (similar to how builtin/intl was filled)
Categories
(Core :: JavaScript: Standard Library, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
Details
Attachments
(16 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
builtin/Stream.*
is something like 5500 lines. If you want to edit it while referring to separate portions of it, the best you can do is multiple editor windows (and hope your editor supports loading some of them readonly or something). If you want to find something in it, it's at least organized by spec section...but still, it's wicked huge.
We should create a new builtin/streams
directory and populate it with a bunch of different files containing all the streams code, split up by spec section or so. The code won't be in literal spec order, but that seems less valuable to me than having things that can be compared easily and kept in mind more easily and where you can't completely lose yourself scrolling through. (Particularly if streams code grows a whole bunch of support for writable streams, piping, readable byte streams, &c. in the future.)
Similarly to what I did in bug 1431957, I have a bunch of patches for this where I have added the necessary using *::*;
for every single symbol each separate file requires -- all the easier to be certain I've properly understood what symbols are depended on and that I'm not accidentally bootlegging something from a place I didn't expect. It shouldn't be terribly difficult to go back through at the end and add using namespace js;
to cull the js::*
symbols that traditionally haven't been singly using
d.
I have a patch stack that takes builtin/Stream.cpp
down to 778 lines, of which more than a third are commented-out code that doesn't build at all. The remaining bits seem mostly to be related to byte streams -- which we do not actually support -- and may not even be close to usable, so presently I'm inclined to just leave them there for now and maybe remove them when actual byte stream support gets added. (But in any event, most of the way is far, far better than trying to clear out every last corner of this.)
Also worth noting -- these patches were developed atop a patch that changed FILES_PER_UNIFIED_FILE=1
, so these files really should be separately compilable pretty safely.
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Depends on D43906
Assignee | ||
Comment 3•5 years ago
|
||
Depends on D43907
Assignee | ||
Comment 4•5 years ago
|
||
Depends on D43908
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D43909
Assignee | ||
Comment 6•5 years ago
|
||
Depends on D43910
Assignee | ||
Comment 7•5 years ago
|
||
Depends on D43911
Assignee | ||
Comment 8•5 years ago
|
||
Depends on D43912
Assignee | ||
Comment 9•5 years ago
|
||
Depends on D43913
Assignee | ||
Comment 10•5 years ago
|
||
Depends on D43914
Assignee | ||
Comment 11•5 years ago
|
||
Depends on D43915
Assignee | ||
Comment 12•5 years ago
|
||
Depends on D43916
Assignee | ||
Comment 13•5 years ago
|
||
Depends on D43917
Assignee | ||
Comment 14•5 years ago
|
||
Depends on D43918
Assignee | ||
Comment 15•5 years ago
|
||
Depends on D43919
Assignee | ||
Comment 16•5 years ago
|
||
Depends on D43920
Updated•5 years ago
|
Assignee | ||
Comment 17•5 years ago
|
||
Sooooooooooo...as a consequence of bug 1577319 I have learned that our nominal style guide claims that using namespace *;
is actually verboten. That bug plans to get rid of all global using namespace std;
to solve its problem. But in the longer term, bigger picture, it seems likely that all using namespace *;
will disappear -- and if so, it may be in our best interests to keep listing out the distinct using
s, rather than me erasing it in a final step.
However -- for now, to reduce the mess of dependencies in my local tree, it's going to be super-helpful for me to be able to land the incremental bits of reviewed code before the whole stack is fully reviewed. And because this namespace thing has some effect on what subsequent revisions touch, I am going to land the patches exactly as they are, without touching anything using namespace
immediately. Just a heads-up.
Comment 18•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/autoland/rev/d55a0cb6238a Rename SetNewList to StoreNewListInFixedSlot, and move it into vm/List-inl.h for use in multiple files. r=arai https://hg.mozilla.org/integration/autoland/rev/e6bd168d9b3d Move queue-with-sizes operations out of builtin/Stream.* into builtin/streams/QueueWithSizes.*. r=arai https://hg.mozilla.org/integration/autoland/rev/468ca4f5c8c6 Rename the streams CLASS_SPEC macro to JS_STREAMS_CLASS_SPEC and move it to its own header. r=arai https://hg.mozilla.org/integration/autoland/rev/3a7342e03942 Move queueing strategies details into builtin/streams/QueueingStrategies.*. r=arai https://hg.mozilla.org/integration/autoland/rev/55e76049ef99 Move various streams miscellaneous operations to a separate file. r=arai
Comment 19•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d55a0cb6238a
https://hg.mozilla.org/mozilla-central/rev/e6bd168d9b3d
https://hg.mozilla.org/mozilla-central/rev/468ca4f5c8c6
https://hg.mozilla.org/mozilla-central/rev/3a7342e03942
https://hg.mozilla.org/mozilla-central/rev/55e76049ef99
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/autoland/rev/724755d6ff65 Move ReadableStream{,Default}Reader details out of builtin/Stream.* to separate files. r=arai https://hg.mozilla.org/integration/autoland/rev/a2c5184de226 Move ReadableStreamReader abstract operations out of Stream.cpp. r=arai https://hg.mozilla.org/integration/autoland/rev/2b9bdb19e36a Move public/friend stream API functions into a new file. r=arai https://hg.mozilla.org/integration/autoland/rev/3f547590647a Move TeeState details into separate files. r=arai https://hg.mozilla.org/integration/autoland/rev/d8ca7b07183c Move ReadableStream abstract operations to their own header. r=arai https://hg.mozilla.org/integration/autoland/rev/e6c27cc40626 Split PullIntoDescriptor code into separate files. r=arai https://hg.mozilla.org/integration/autoland/rev/035b23218006 Move ReadableStream standard library bits into separate files. r=arai https://hg.mozilla.org/integration/autoland/rev/b1148ccfa82f Move IsMaybeWrapped to a header so it can be used in multiple files. (It can't be *static* in those multiple files because of scumbag unified builds.) r=arai https://hg.mozilla.org/integration/autoland/rev/5aa0ce936427 Move ReadableStream stream/controller interactions to separate files. r=arai https://hg.mozilla.org/integration/autoland/rev/50eea29e2a96 Move ReadableStreamDefaultController details into separate files. r=arai https://hg.mozilla.org/integration/autoland/rev/d42177dc73dc Move readable stream default controller operations into their own file. r=arai
Comment 21•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/724755d6ff65
https://hg.mozilla.org/mozilla-central/rev/a2c5184de226
https://hg.mozilla.org/mozilla-central/rev/2b9bdb19e36a
https://hg.mozilla.org/mozilla-central/rev/3f547590647a
https://hg.mozilla.org/mozilla-central/rev/d8ca7b07183c
https://hg.mozilla.org/mozilla-central/rev/e6c27cc40626
https://hg.mozilla.org/mozilla-central/rev/035b23218006
https://hg.mozilla.org/mozilla-central/rev/b1148ccfa82f
https://hg.mozilla.org/mozilla-central/rev/5aa0ce936427
https://hg.mozilla.org/mozilla-central/rev/50eea29e2a96
https://hg.mozilla.org/mozilla-central/rev/d42177dc73dc
Comment 22•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/d55a0cb6238a
https://hg.mozilla.org/releases/mozilla-beta/rev/e6bd168d9b3d
https://hg.mozilla.org/releases/mozilla-beta/rev/468ca4f5c8c6
https://hg.mozilla.org/releases/mozilla-beta/rev/3a7342e03942
https://hg.mozilla.org/releases/mozilla-beta/rev/55e76049ef99
https://hg.mozilla.org/releases/mozilla-beta/rev/724755d6ff65
https://hg.mozilla.org/releases/mozilla-beta/rev/a2c5184de226
https://hg.mozilla.org/releases/mozilla-beta/rev/2b9bdb19e36a
https://hg.mozilla.org/releases/mozilla-beta/rev/3f547590647a
https://hg.mozilla.org/releases/mozilla-beta/rev/d8ca7b07183c
https://hg.mozilla.org/releases/mozilla-beta/rev/e6c27cc40626
https://hg.mozilla.org/releases/mozilla-beta/rev/035b23218006
https://hg.mozilla.org/releases/mozilla-beta/rev/b1148ccfa82f
https://hg.mozilla.org/releases/mozilla-beta/rev/5aa0ce936427
https://hg.mozilla.org/releases/mozilla-beta/rev/50eea29e2a96
https://hg.mozilla.org/releases/mozilla-beta/rev/d42177dc73dc
Description
•