Hotfix/issue #1565 sleep private properties #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ot contain leading `\0`
…te props are requested
Ocramius
pushed a commit
that referenced
this pull request
Mar 9, 2014
The per-opcode abstract interpreter code was getting too
large to reasonably be in a single file. Getting minstr stuff out on
its own seemed overdue, and all the helpers (popC(), etc) could also
go somewhere else. Later we might even want to separate out a few
other groups (e.g. arithmetic: the type-arith functions have no way
right now to indicate when something is nothrow, but I was definitely
not going to add them to the InterpStepper class just to allow that).
This is a long commit message, but reviewing the rationale is probably
more relevant than reviewing the code.
I considered the following solutions:
- 1. Leave it all in a class, but define different functions in
different .cpps. (I started down this path and then threw it
away.)
- 2. Still have one translation unit, but #include different
sections from different files.
- 3. Make everything non-members, but use macros to make a mini-DSL
for accessing state that still works with implicit parameters.
(Define opcode functions with something like IOP(CGetM) {}, etc.)
The main problem I have with #1 is that it increases the friction for
adding helper methods. In HHBBC the total code size is still pretty
small, so it's not a huge deal to recompile everything when you change
interp.h. But this doesn't scale, and elsewhere in the codebase where
we have objects with one method per bytecode I think the increased
penalty to adding helpers has increased function size, so this is a
problem worth thinking about more generally. (In particular, if you
want to add a helper to bytecode.cpp, you often have to declare it in
a header that will cause you rebuild the whole world. See #3834388.)
The facebook#2 idea seems plausible, but weird. Its impact on build times is
hard to predict (and might even be good in some cases), but its impact
on implementation hiding is negative: you can't declare helpers in an
unnamed namespace and be sure they are private.
The facebook#3 one bothers me because it seems really unfortunate to define
names like "popC" in ways that don't respect scope. A few of them are
plausibly identifiers you might want to have active for more than one
purpose in some of these files. The cost of passing "env" everywhere
(a common idiom in ML) seems arguably lower than that. It seems
plausible that there may be some cases where this option is
preferable, though.
After starting down the path of #1, and then some thinking, I'm of the
opinion that putting all the methods in the same object isn't really
providing us the intended benefit in cases like this. It seems that
the reason you want to put methods like this in the same class is so
you can declare some of the state private, and hide it from the rest
of the program. However, in cases like this (or bytecode.cpp), member
accessibility is not really being used to provide an abstraction
layer. And for hiding methods, you get much better hiding if you can
declare them local to .cpp files instead of private to a class.
So the solution I ended up with is to just pass a parameter to each
method manually. This means any function can be defined in any
translation unit that we want. Also, data hiding is actually not any
worse than the class approach: the interpreter functions can still
only modify versions of the state that they are given access to (by
actually passing it a pointer), which was exactly the state they had
access to before this, since these structures live on the stack and
can't be reached any other way.
One detail specific to this diff: there is a family of instructions
with special additional state (the minstr instructions), but they need
to call all the generic functions as well (popC, locAsCell, etc).
This diff implements this by making the minstr environment struct a
subtype of the general interpreter environment, so you still pass a
single state object everywhere, and it's legal to call popC(env) when
`env' is a minstr env.
Reviewed By: @edwinsmith
Differential Revision: D1200426
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.