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

Adds IO::Memory.truncate() #6459

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

chris-huxtable
Copy link
Contributor

@chris-huxtable chris-huxtable commented Jul 27, 2018

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.

spec/std/io/memory_spec.cr Outdated Show resolved Hide resolved
src/io/memory.cr Outdated Show resolved Hide resolved
src/io/memory.cr Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor

Use cases?

@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Jul 27, 2018

I originally was using it as a monkey patch for my console singleton. I needed to add an equivalent to "\b \b" for an IO:Memory backed buffer. I am sure there are more.

@RX14
Copy link
Contributor

RX14 commented Jul 30, 2018

This is fine, but there's already a File#truncate, and that method takes a size and a default which is zero. It'd be good to keep these two method's signatures they same considering they're both IOs.

Probably best to add an explicit size option to IO::Memory's truncate, and then make a breaking change to set the default for File's truncate to pos.

@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Jul 31, 2018

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")

What do you think about having it take an optional IO::Memory::Seek so it can still be relative to the position if specified?

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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note -> NOTE

Copy link
Contributor Author

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.

@RX14
Copy link
Contributor

RX14 commented Jul 31, 2018

I really want File#truncate and IO::Memory#truncate to behave as similar as possible. This means being able to extend it past the current size by padding with zeroes. Also supporting negative sizes in File#truncate.

@chris-huxtable
Copy link
Contributor Author

I can understand the negative values, I have already implemented them for IO::Memory and will for File but I am not a fan of using truncate to extend the size. Perhaps a method like extend or resize?

@asterite
Copy link
Member

Note that setting pos of an IO::Memory past its size, and then writing to it, will fill the gap with \0. This is similar to what happens with a File, and with Ruby's StringIO. So these behave the same in Crystal:

# 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 \0:

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 IO::Memory.

By the way, Ruby's StringIO implements truncate and also behaves like that.

@RX14
Copy link
Contributor

RX14 commented Jul 31, 2018

ftruncate is the syscall to extend the size of a file from a filedescriptor, maybe we should name it resize, or just #size=, who knows. I actually like the #size= API because then you can do size += and size -= to naturally express differences.

@chris-huxtable
Copy link
Contributor Author

While working on this I noticed the growth factor is 2, which is arguably the worst choice. Based on this article and what NSMutableArray uses internally I propose 1.625.

@asterite, @ysbaddaden - Thoughts?

@ysbaddaden
Copy link
Contributor

No opinion, that depends on the memory allocator and growth. With my GC you'll never reuse the previous allocation anyway.

@asterite
Copy link
Member

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.

@chris-huxtable
Copy link
Contributor Author

I will revert the growth factor and do some benchmarking. If that all works out I’ll open a new PR.

@chris-huxtable
Copy link
Contributor Author

Ready for review.

Copy link
Member

@straight-shoota straight-shoota left a 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
Copy link
Member

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
Copy link
Member

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")
Copy link
Member

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.

Copy link
Contributor

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.

@chris-huxtable
Copy link
Contributor Author

@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.

@straight-shoota straight-shoota added the pr:needs-work A PR requires modifications by the author. label Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature pr:needs-work A PR requires modifications by the author. topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants