-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Add Pathname#empty?
#4322
Add Pathname#empty?
#4322
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bigger change and a smaller change
@@ -444,7 +444,7 @@ public static RubyBoolean writable_p(IRubyObject recv, IRubyObject filename) { | |||
return RubyFileTest.writable_p(recv, filename); | |||
} | |||
|
|||
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our style is to have '} else {' instead of having closing brackets on their own lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOH. I should have clicked request changes...too fast to click buttons :)
…od`. Fix `else` style problem.
Pathname#empty?
empty?
implementation onFile
sinceFileTest
will 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