Refactoring module_function
#4931
Conversation
There was a problem hiding this comment.
I think this change is clean but I do not think we want to pay resetting an additional field in Frame. This will have some measurable cost. We are trying to always prune down cost of frame management.
I wonder if we can somehow combine state of capture and this boolean together possibly something as icky as an Enum although maybe a module_function frame can also capture? bit flags combining would be another idea since neither flag is set by default. Just thinking out loud...
Remove `MODULE_FUNCTION` from `Visibility` and
add `isModuleFunction` to `Frame`.
Implementing `module_function` as `Visibility`
makes the visibility of `foo` to be `MODULE_FUNCTION`:
```
Module.new do
module_function
define_method(:foo) {:foo}
end
```
`module_function` is not a type of visibility
but states of frames. So change to manage it
on `Frame`.
yui-knk
force-pushed
the
test_define_method_visibility_2
branch
from
97e89a6 to
6a9aa24
Compare
Introduce `FrameVisibility` to handle visibility of methods and frames separately.
|
I introduced |
|
I want to revert last commit, because last commit breaks many test cases... |
|
@yui-knk do you know why they failed? Removal of that enum value? |
enebo
dismissed
their stale review
We need to resolve what broke from this PR
|
@yui-knk I am unsure from reading comments what is needed to move forward on this PR? |
Remove
MODULE_FUNCTIONfromVisibilityandadd
isModuleFunctiontoFrame.Implementing
module_functionasVisibilitymakes the visibility of
footo beMODULE_FUNCTION:module_functionis not a type of visibilitybut states of frames. So change to manage it
on
Frame.