-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Adds IO::Memory.truncate()
#6459
base: master
Are you sure you want to change the base?
Adds IO::Memory.truncate()
#6459
Conversation
Use cases? |
I originally was using it as a monkey patch for my console singleton. I needed to add an equivalent to "\b \b" for an |
This is fine, but there's already a Probably best to add an explicit |
It makes no sense to have a negative size. I will be making a negative value similar to a negative value in a range. In other words io = IO::Memory.new()
io << "hello world"
io.truncate(-6)
io.pos = 0
io.gets().should eq("hello")
|
Is now similar to `File.truncate`
src/io/memory.cr
Outdated
@@ -336,6 +336,23 @@ class IO::Memory < IO | |||
@pos = value.to_i | |||
end | |||
|
|||
# Truncates the memory to the specified *size*. | |||
# | |||
# Note: a negative value truncates from the end of the `IO::Memory`. |
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.
Note
-> NOTE
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.
Its used in both ways throughout the project. I will use the caps as it doesn't really matter to me and seems more consistently applied.
I really want |
I can understand the negative values, I have already implemented them for |
Note that setting # Using a file
io = File.open("test.txt", "w+")
io.pos = 3
io.puts 1
io.rewind
p io.gets_to_end # => "\u0000\u0000\u00001\n"
io.close
# Using an IO::Memory
io = IO::Memory.new
io.pos = 3
io.puts 1
io.rewind
p io.gets_to_end # => "\u0000\u0000\u00001\n"
io.close Truncating a file past its size (or well, past its maximum current size) fills that with io = File.open("test.txt", "w+")
io.truncate(5)
io.puts 1
io.rewind
p io.gets_to_end # => "1\n\u0000\u0000\u0000"
io.close and the same should happen with By the way, Ruby's |
|
While working on this I noticed the growth factor is 2, which is arguably the worst choice. Based on this article and what @asterite, @ysbaddaden - Thoughts? |
No opinion, that depends on the memory allocator and growth. With my GC you'll never reuse the previous allocation anyway. |
Please revert the growth factor change. You just made your PR unmergable because we now must discuss two things instead of one. Please open a new issue or PR to discuss the growth factor change, which should also be applied to arrays, maybe. |
I will revert the growth factor and do some benchmarking. If that all works out I’ll open a new PR. |
75ddf16
to
6504371
Compare
Ready for review. |
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.
Please remove "can" from the spec descriptions, that's just superfluous information.
The specs could also be grouped in a describe "#truncate"
.
# Truncates the memory to the specified *size*. | ||
# | ||
# NOTE: a negative value truncates from the end of the `IO::Memory`. | ||
def truncate(size = 0) : Nil |
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.
I think size
should be restricted to Int, or better even Int32.
io.to_s.should eq("hello") | ||
end | ||
|
||
it "raises when truncate size is too large" do |
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 description is incorrect. There is no raise.
io.truncate(12) | ||
io.truncate(13) | ||
|
||
io.to_s.should eq("hello world\u0000\u0000") |
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.
Should truncating with size > bytesize
really fill up with zero bytes?
This is not truncating but expanding and the semantic difference is only governed by the current bytesize.
I'd rather have it fault tolerant and don't grow bytesize on truncate.
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.
bytesize
growing on truncate matches the behavior of File
.
@straight-shoota - Thanks for the review. I am absolutely slammed with work ATM. I won't be able to take a stab at this for a little while. |
Truncates the contents of IO::Memory at the current cursor position.
Thought this might be a useful addition. Seeing as we have
.clear()
and this is just a special variation.