-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] returnIDs use Objects, Thread#raise, minor updates #3140
Conversation
Truffle::Primitive.thread_raise self, exc | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this code come from? If it is modified it should be in api/shims.
If we can work with the original code, it is usually better, so in this case we could implement the thread_raise
Rubinius primitive, right? (In ThreadPrimitiveNodes) See VMPrimitiveNodes
for an example of Rubinius primitives nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original Rubinius Thread#raise
uses Rubinius.lock
and other intrinsic @alive
variable, checks if the Thread is alive etc. That's the reason why this method is modified, so moving to shims. Does it makes sense to use Rubinius primitive if the ruby wrapping code is different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think having the Rubinius primitive still makes sense to preserve most of the original code (and also better organization, not visible so directly to user, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Looks great, please address my 3 comments. |
@CoreMethod(names = "thread_raise", required = 2, onSingleton = true) | ||
public abstract static class RaiseNode extends CoreMethodArrayArgumentsNode { | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be careful about things like random extra newlines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, IDE formatter configuration updated.
08d32c3
to
892f039
Compare
raise_prim exc | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this should be in a separate file, because the loading order will load Rubinius' common/thread.rb after this according to core.rb.
The solution is to move it to a separate file, for instance named thread_common.rb
or thread_after_common.rb
and add that one after common/thread,rb
in core.rb.
We should have a better organization for all of this.
cc @nirvdrum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small correction Thread#raise
is defined in bootstrap/thread
we do not have it defined in our repo though, which was to reason why I've putted into existing shims/thread
. You are right that it's error prone (if we later copy the bootsrap
) and it would be better to have it loaded in extra file. Moving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so that's why the CI passes :) Indeed, the more clarity the better!
The comments were deleted when new changes were pushed, so I think I only have part of the conversation from email. But, if you want |
@nirvdrum I've used just part of the original method, that's why the discussion where to put it. The original method used internal implementation details. I did not import the original bootstrap method, should I? |
@pitr-ch That's fine. If you find yourself needing to only exclude part of a method |
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
# | ||
# Modifications are subject to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you've added any code (i.e., didn't just delete stuff from Rubinius), you really don't need this. And even then, it's questionable for trivial changes. There's no harm in having it here, but it's not usually a requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But large parts of both methods below are part of Rubinius, so the license should be in this file, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The Rubinius license should be in tact. I specifically meant the modification parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few minor additions, so I'll keep it in this case.
Another possible approach to this could be to always copy the original method with the rubinius license and have the overrides in shims with truffle license. Then it would not be mixed and we would be able to say easily what is the difference. What do you think?
FYI, I think what you did with the new shims file is fine. As @eregon said, now that we have a pattern of a few use cases, we should tidy up. Most of the files in api/shims really aren't shims for an API. |
@nirvdrum Yeah, I think actually most of "API shims" are really just defined in bootstrap in proper Rubinius and it would make more sense to define it with our files at that same bootstrap stage. |
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
# | ||
# Modifications are subject to: | ||
# Copyright (c) 2014, 2015 Oracle and/or its affiliates. All rights reserved. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you ported this from elsewhere, the code should not be copyrighted in 2014.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point it should be just 2015
yeah, right now the Regarding this, PR can I merge? |
Not today! We're waiting for the 9k release. |
@pitr-ch sorry couldn't merge this. Can you pull, fix and merge? |
@chrisseaton np, after ci passes i'll merge |
It was giving bad rusults for cases like: {a:1, b:2}.merge(b:3) # => {:a=>1, :b=>2}
* to be able to get the backtrace in debuger
from core/rubinius/boostrap/thread to core/rubinius/api/shims/thread
…tive it should return Time instance only when called in class Time or a descendant class RSpec::Core::Time class << self define_method(:now, &::Time.method(:now)) end end RSpec::Core::Time.now was returning RSpec::Core::Time instence instead of Time
No description provided.