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

__dir__ won't work with embed paths such as uri:classloader: (#4611) #4658

Merged
merged 3 commits into from Jun 12, 2017

Conversation

Projects
None yet
3 participants
@kares
Member

kares commented Jun 9, 2017

... if File#getAbsolutePath() is used - which seems like its not necessary
and we can pretty much just do what MRI does File.dirname(__FILE__)

wonder if caller is actually necessary in __dir__ // cc @headius @enebo

as adviced in #4611 (comment)

@kares I think we need the same special-case logic here, or better, make this call hopefully-existing logic that does the right thing already.

... this is relying on the existing uri:classloader: handling logic in RubyFile.dirname

verified this works fine for liquid gem's case presented in #4611

still need to figure out a where to put a test-case to test this properly

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 9, 2017

Member

The caller logic needs to be there because jitted methods don't set context.getFile to anything. This is obviously unfortunate since it requires generating a stack trace and then mining the filename out of it.

A future improvement would be to mark "file" as a frame field needed for code that calls anything like "dir". It would force a frame and force setting file so that downstream calls could pick it up.

Another would be to implement dir as a pseudo-keyword, not making a call if we know it's the builtin one, so it can access the filename of the current method directly. dir would essentially compile down to dir_method_builtin ? File.dirname(__FILE__) : call_dir_method.

PR looks good to me!

Member

headius commented Jun 9, 2017

The caller logic needs to be there because jitted methods don't set context.getFile to anything. This is obviously unfortunate since it requires generating a stack trace and then mining the filename out of it.

A future improvement would be to mark "file" as a frame field needed for code that calls anything like "dir". It would force a frame and force setting file so that downstream calls could pick it up.

Another would be to implement dir as a pseudo-keyword, not making a call if we know it's the builtin one, so it can access the filename of the current method directly. dir would essentially compile down to dir_method_builtin ? File.dirname(__FILE__) : call_dir_method.

PR looks good to me!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 9, 2017

Member

Oops, I spoke too soon...I didn't realize you were removing the caller logic.

So caller logic doesn't work because our compiler doesn't embed the full uri:classloader stuff as the name, or something?

Member

headius commented Jun 9, 2017

Oops, I spoke too soon...I didn't realize you were removing the caller logic.

So caller logic doesn't work because our compiler doesn't embed the full uri:classloader stuff as the name, or something?

Show outdated Hide outdated core/src/main/java/org/jruby/RubyKernel.java Outdated
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 9, 2017

Member

ah, I see ... that's unfortunate. I'll put the caller gathering back than. thanks.

Member

kares commented Jun 9, 2017

ah, I see ... that's unfortunate. I'll put the caller gathering back than. thanks.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jun 11, 2017

Member

caller logic is back and I have also resurrected test_jar_complete.rb on master so this can be tested.

Member

kares commented Jun 11, 2017

caller logic is back and I have also resurrected test_jar_complete.rb on master so this can be tested.

@kares kares added this to the JRuby 9.1.11.0 milestone Jun 11, 2017

kares added some commits Jun 9, 2017

@kares kares merged commit 0190ec8 into jruby:master Jun 12, 2017

1 check passed

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

@hiroyuki-sato hiroyuki-sato referenced this pull request Jun 13, 2017

Closed

Update Liquid 4.0.0 again. #637

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment