-
Notifications
You must be signed in to change notification settings - Fork 550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement an optional mirror
module.
#1006
base: main
Are you sure you want to change the base?
Conversation
Thought: Might be easier to review/read the changes if all the makefile auto-built changes weren't also included? |
Yes and no. First patch is an utility patch, second patch is the core of the patch, and third one is the premake update in isolation. And personally, while I understand the motivations behind the inclusion of the generated files, their inclusion is a huge pain as it creates a fragmentation for contributions. I never know how to push changes that impacts premake generated files, because of differences between official and my local premake versions, and the fact it is required for the project build. |
Ah, true your commit history is indeed very clean. That's not always the case (in general - perhaps it's better here in Wren) and I'm just so accustomed to using "Files changed" for reviews. :-) Agree entirely with the "huge pain" comment... although it was a small pain to get the correct version of |
Ignoring some changes that I developed in separated branches that I don't consider urgent for inclusion, my main branch is +200 patch ahead of main. So I'm "forced" to tidy my changes, so (check) rebasing is as seamless as possible. |
Fixed a some issues I noticed. added patch to access |
Arff changed my mind, will have to have a minimal layer of |
Since we don't expose And I still think we should put it in the Also, before we merge this PR, I think we need to have some way to actually invoke methods. I have some ideas, and would like to think about it a bit more. Finally, what's the purpose of |
setSlot must renamed, since it is not static anymore, the symbol will be
leaked in the library.
MethodMirror is still a place holder as I hack the thing. eval is much more
dangerous than the mirror API, and I want to be able to disable it.
|
4ac3196
to
c8edbb5
Compare
The
|
While I agree, for now it is not possible because there is no pointer from the method to the class. I have to look how it impacts the vm. |
The only thing I needed to add: class StackTrace {
frames { _stackTrace } // added For accessing the raw frames so you can process them however you may want. |
I have to think about that, because that way you can alter the stack trace, which is what I wanted to forbid. |
So why not? frames { _stackTrace[0..-1] } |
Was more thinking at making it a readonly list like container. |
To me it's just a list of items... once I have it I should be able to alter it, remove items, insert new items, it's MY list.. as long as I have a copy and I'm not messing with the canonical version or something bad. |
4f1cb3e
to
6157db1
Compare
I updated so you should be able to pull the class form the For your last concern, you can do |
Avoid to update yet, I have some concerns about how |
a22c940
to
c7dbcd2
Compare
Updated. Not final yet, but in better shape, and allows "MethodMirror" to perform calls (though it needs a little bit more security). |
Silenced an issue, where method can be bound multiple times. While the grammar allow inner class definitions and the behavior is even tested, it is implemented incorrectly. For that behavior to be correct; the whole |
Fixed silenced issue thinko. |
An attempt at implementing #1004, using a mirror module.
Not yet ready for merge.