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

Ruby 2.7: Beginless ranges fully supported #2211

Merged
merged 12 commits into from Jan 12, 2021
Merged

Conversation

LillianZ
Copy link
Contributor

@LillianZ LillianZ commented Jan 6, 2021

Finishing up from #2155

In this PR, added support for:
Range#{cover?, include?, ===}.
String#{byteslice, slice, slice!} and Symbol#slice
Kernel#{caller, caller_locations} and Thread#backtrace_locations

Shopify#1

@eregon
Copy link
Member

eregon commented Jan 7, 2021

I'll let @norswap review the rest.

Copy link
Contributor

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Looks good. Great work on all these new specs 👍

src/main/ruby/truffleruby/core/truffle/range_operations.rb Outdated Show resolved Hide resolved
unless beg_compare
return false if range.begin # not beginless
beg_compare = -1
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as cover_range?, hard to follow because of the existing code.

Proposal (completely untested):

def self.cover?(range, value)
  # Check lower bound.
  if !Primitive.nil?(range.begin)
    # MRI uses <=> to compare, so must we.
    cmp = (range.begin <=> value)
    return false if Compare.compare_int(cmp) > 0
  end

  # Check upper bound.
  if !Primitive.nil(range.end)
    value = value + (range.exclude_end? 1 : 0)
    cmp = (value <=> range.end)
    return false if Comparable.compare_int(cmp) > 0
  end

  true
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also much more beautiful, thanks!

Also, r_less is no longer being used, so I commented it out; not sure if it should be removed altogether.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's remove range_less if it's unused :)
(we can always use the git history to restore it if it's useful in the future)

Copy link
Contributor

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Seems good to go for me, unless there are more changes that need to go in?

@LillianZ
Copy link
Contributor Author

LillianZ commented Jan 8, 2021

I'm done and ready to merge. Thank you for the great suggestions!

@chrisseaton
Copy link
Collaborator

Great PR to finish a tricky set of specs!

@eregon
Copy link
Member

eregon commented Jan 9, 2021

There has some CI failures, I'm not sure why GitHub Actions didn't run on the last changes.
In MRI tests (same in some specs):


  1) Failure:
Bug::Test_ExceptionAT#test_exception_at_throwing [/b/b/e/main/test/mri/tests/cext-ruby/exception/test_exception_at_throwing.rb:7]:
assert_separately failed with error message
pid 22634 exit 0
| resource:/truffleruby/core/truffle/range_operations.rb:93: warning: `-' after local variable or literal is interpreted as binary operator
| resource:/truffleruby/core/truffle/range_operations.rb:93: warning: even though it seems like unary operator


  2) Failure:
OpenSSL::TestEngine#test_engines_free [/b/b/e/main/test/mri/tests/openssl/test_engine.rb:76]:
assert_separately failed with error message
pid 22838 exit 0
| resource:/truffleruby/core/truffle/range_operations.rb:93: warning: `-' after local variable or literal is interpreted as binary operator
| resource:/truffleruby/core/truffle/range_operations.rb:93: warning: even though it seems like unary operator

Lint:

Offenses:

src/main/ruby/truffleruby/core/truffle/range_operations.rb:85:68: C: Layout/TrailingWhitespace: Trailing whitespace detected.
      return false unless (Primitive.nil?(b2) || cover?(range, b2)) 
                                                                   ^

@LillianZ
Copy link
Contributor Author

Thanks for letting me know, I think it's fixed now!

The non-running tests could have something to do with the fact the CHANGELOG is conflicting? In the future I'll wait until after the reviews to write it.

@eregon
Copy link
Member

eregon commented Jan 12, 2021

The non-running tests could have something to do with the fact the CHANGELOG is conflicting?

That seems likely, I didn't think about it.
Something worth mentioning in that GitHub issue about an incorrectly-reported conflict, cc @chrisseaton

A workaround would be to rebase the PR, but we'll try first if this now passes the internal CI.

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Jan 12, 2021
graalvmbot pushed a commit that referenced this pull request Jan 12, 2021
@graalvmbot graalvmbot merged commit faaeba6 into oracle:master Jan 12, 2021
@LillianZ LillianZ deleted the beginless branch January 12, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants