Conversation
* Add `Pathname#empty?` * Remove `empty?` implementation on `File` since `FileTest` will assign its implementation of `empty?` to `File` (see `RubyFileTest.FileTestFileMethods`) * Add `FileTest.empty?` The last two steps were down in this PR in order to base the implementation of `Pathname#empty?` on the MRI implementation. See jruby#4293
enebo
left a comment
enebo
left a comment
There was a problem hiding this comment.
A bigger change and a smaller change
| @JRubyMethod(name = "zero?", required = 1) | ||
| @JRubyMethod(name = {"empty?", "zero?"}, required = 1) | ||
| public static RubyBoolean zero_p(ThreadContext context, IRubyObject recv, IRubyObject filename) { | ||
| return RubyFileTest.zero_p(context, recv, filename); |
There was a problem hiding this comment.
This might work but I can see in MRI source that there is a defintion for zero? in FileTest and one in Dir. Can you add a definition to RubyDir and then remove the directory? dynamic dispatch path. If somehow a Dir does end up calling the FileTest version (like replace Dir#empty? with a def which calls super) then the stat call I think will still work.
There was a problem hiding this comment.
oh another thing to mention is that we try not to add dynamic dispatches when we can help it and this will make ordinary file zero? tests more expensive (due to dyn dispatch and the second stat call).
There was a problem hiding this comment.
@enebo I pushed a new commit that uses direct calls to RubyFileTest and RubyDir instead of callMethod. Is that what you where looking for? I didn't think I needed to "add a definition to RubyDir" because there is already one there for empty? (the method empty_p).
Also, by dynamic dispatch, are you referring to my use of callMethod? That's what I was thinking you were referring to.
Thanks in advance for your feedback. It has been helpful so far.
There was a problem hiding this comment.
@kcdragon shoot. I messed up! I thought your deifnition of zero_p was on RubyFileTest and not on RubyPathname. MRI does do a dynamic call (callMethod or rb_funcall in MRI) in pathname. sorry for messing that up. You had it right the first time :|
There was a problem hiding this comment.
@enebo Ok. No big deal. I undid my most recent changes but kept the change for else style (} else {) in. Let me know if there is anything else.
| if (fileTest.callMethod(context, "directory?", getPath()).isTrue()) { | ||
| return context.runtime.getDir().callMethod(context, "empty?", getPath()); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Our style is to have '} else {' instead of having closing brackets on their own lines.
There was a problem hiding this comment.
DOH. I should have clicked request changes...too fast to click buttons :)
…od`. Fix `else` style problem.
2 participants
Pathname#empty?empty?implementation onFilesinceFileTestwill assign its implementation ofempty?toFile(seeRubyFileTest.FileTestFileMethods)FileTest.empty?The last two steps were down in this PR in order to base the implementation of
Pathname#empty?on the MRI implementation (which usesDir.empty?andFileTest.empty?. see https://ruby-doc.org/stdlib-2.4.0_preview3/libdoc/pathname/rdoc/Pathname.html#method-i-empty-3F).This PR gets the failing test in "test/mri/pathname/test_pathname.rb" to pass. The tests in "test/mri/ruby/test_file_exhaustive.rb" are also still passing after the changes to
File.See #4293