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

remove getClass from RubyRegexp #5410

Merged
merged 1 commit into from Nov 16, 2018

Conversation

Projects
None yet
3 participants
@ahorek
Copy link
Contributor

ahorek commented Nov 3, 2018

dead code?

pavel

@ahorek ahorek force-pushed the ahorek:dead_code branch from 77b6a9c to 645c2b9 Nov 3, 2018

@ahorek ahorek changed the title WIP: remove RubyRegexp#getClass remove RubyRegexp#getClass Nov 3, 2018

@ahorek ahorek changed the title remove RubyRegexp#getClass remove getClass from RubyRegexp Nov 3, 2018

@kares

This comment has been minimized.

Copy link
Member

kares commented Nov 16, 2018

no response from authors ... so let's kill it, shall we 😈

@kares kares merged commit a0fc962 into jruby:master Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kares kares added this to the JRuby 9.2.5.0 milestone Nov 16, 2018

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

I think these are null checks. It's a pattern I don't agree with, but getClass gets intrinsified to some fast check that's less expensive than doing == null I think. Obviously hard to know that without comments, though 🤔

@ahorek @kares I think we should restore these with comments or just make them old-fashioned null checks.

@kares

This comment has been minimized.

Copy link
Member

kares commented Nov 20, 2018

in a way they were is just weird, most of them will fail with a NPE further down the path.
for those wout an explicit use (maybe one), maybe why not just have an assert bytes != null ?

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Nov 20, 2018

@headius could you add a test case to confirm these checks are necessary? assert != null sounds better...

headius added a commit that referenced this pull request Nov 20, 2018

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

I went ahead with the asserts in 84be473.

@ahorek It may be possible to prove these never receive null but I figured we should put the checks back for now anyway. Obviously these have not been firing for normal JRuby users or we'd have seen the NPE bug reports. Asserts add to bytecode size but will not execute normally, so this seems ok. And these do need to be be non-null, so having the asserts is appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.