Skip to content
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

Merged
merged 19 commits into from
Jul 23, 2015

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented Jul 15, 2015

No description provided.

@pitr-ch pitr-ch self-assigned this Jul 15, 2015
@pitr-ch pitr-ch added this to the truffle-dev milestone Jul 15, 2015
Truffle::Primitive.thread_raise self, exc
end
end

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@eregon
Copy link
Member

eregon commented Jul 16, 2015

Looks great, please address my 3 comments.

@CoreMethod(names = "thread_raise", required = 2, onSingleton = true)
public abstract static class RaiseNode extends CoreMethodArrayArgumentsNode {


Copy link
Contributor

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.

Copy link
Member Author

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.

@pitr-ch pitr-ch force-pushed the master branch 3 times, most recently from 08d32c3 to 892f039 Compare July 18, 2015 12:29
raise_prim exc
end
end

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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!

@nirvdrum
Copy link
Contributor

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 Thread.raise from the Rubinius bootstrap file (and don't need to change it), just import it from the correct version of Rubinius. The bootstrap files have been treated as additive, while the core files are 100% what upstream has and we exclude some stuff we can't use.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jul 20, 2015

@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?

@nirvdrum
Copy link
Contributor

@pitr-ch That's fine. If you find yourself needing to only exclude part of a method Truffle.omit is an option as well (grep for current usages).

# 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:
Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member Author

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?

@nirvdrum
Copy link
Contributor

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.

@eregon
Copy link
Member

eregon commented Jul 20, 2015

@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
Copy link
Contributor

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.

Copy link
Member Author

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

@pitr-ch
Copy link
Member Author

pitr-ch commented Jul 20, 2015

yeah, right now the core.rb serves as a source to find out where the file belongs. We could have shims/ followed by the same structure of directories as rubinus has for its files.

Regarding this, PR can I merge?

@chrisseaton
Copy link
Contributor

Regarding this, PR can I merge?

Not today! We're waiting for the 9k release.

@chrisseaton
Copy link
Contributor

@pitr-ch sorry couldn't merge this. Can you pull, fix and merge?

@pitr-ch
Copy link
Member Author

pitr-ch commented Jul 23, 2015

@chrisseaton np, after ci passes i'll merge

pitr-ch added 2 commits July 23, 2015 18:06
It was giving bad rusults for cases like:
{a:1, b:2}.merge(b:3) # => {:a=>1, :b=>2}
pitr-ch added 17 commits July 23, 2015 18:06
* 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
@pitr-ch pitr-ch merged commit f38195d into jruby:master Jul 23, 2015
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants