Bug #18417
Updated by zverok (Victor Shepelev) over 2 years ago
Hello. While working for documentation for `IO::Buffer` (still WIP), I uncovered these problems. I believe at least some of them should be fixed before the release, though I understand not much time left.
The list is not final, probably (still on it, but wanted to raise awareness ASAP).
**These two should be probably fixed/discussed before the release**
1\. I believe `to_str` is an unfortunate name for "read string from buffer, maybe entire buffer. It is Ruby's convention that `to_str` is an implicit String-conversion, and a) I am not sure the Buffer is "string-like" enough to be implicitly converted, and b) it is really unexpected that `#to_s` and `#to_str` do different things:
```ruby
buf = IO::Buffer.for('test')
"buf: #{buf}"
# => "buf: #<IO::Buffer 0x00007fdb7f4e3a20+4 SLICE>"
"buf: " + buf
# => "buf: test"
puts buf
# #<IO::Buffer 0x00007fdb7f4e3a20+4 SLICE>
puts '' + buf
# test
```
Another concern about `#to_str` naming is it is one of the main parts of the buffer interface (alongside `#copy`), but doesn't look this way, and looks like some utility method instead. Maybe copy/to_str pair should be symmetrical, like `write`/`read`, or something like that?
2\. I am not sure that type names (`:U8`, `:S8`, `:U16`, `:u16` and so on) introduced "from scratch" (?) is a good thing. From one point, they are kinda nice, and consistent. From another, we already have abbreviated names for the types in [Array#pack](https://docs.ruby-lang.org/en/master/Array.html#method-i-pack), and having two completely unrelated sets of designation for the same thing in the **core of the language** seems weird to me. I understand that not all `#pack`s designations can be represented by Symbol, and their consistency is questionable (it is `S` for 16-bit unsigned LE, `s` for 16-bit signed LE, and `S>`/`s>` for the same in BE), but the discrepancy is questionable too.
3\. About `flags` as a creation parameter:
* `::new`: it seems that the only acceptable value is `MAPPED`; of others, `INTERNAL` is the default, `EXTERNAL`, `IMMUTABLE` and `LOCKED` raise on creation;
* `::for`: no flags accepted (though it seems that a method to make such buffer immutable might be of some utility?)
* `::map`: `INTERNAL`, `LOCKED`, `EXTERNAL` are ignored, `MAPPED` is the default; seems like the only usage of the parameter is _not_ to pass `IMMUTABLE` which is the default: and for that, you'll need to also pass `size` and `offset`.
It seems, that **we shouldn't probably expose "flags" concept to the user** at all, and instead make APIs like `::new(size, mapped: false)` and `::map(io, ..., immutable: true)`?
**Probably bugs**
* It seems that `#clear` behaves weirdly when `offset` is provided but `length` is not:
```ruby
buf = IO::Buffer.new(4)
buf.clear(1, 2) # it seems obvious that I want to clear from byte 2 to the end of the buffer
# in `clear': Offset + length out of bounds! (RuntimeError)
```
* `IO::Buffer.map('README.md')` (just a string, I experimented "whether it is smart enough") core dumps.
* I suspect we might want to set buffer to `IMMUTABLE` if it is created from the frozen string. Currently, this works:
```ruby
str = 'test'.freeze
buf = IO::Buffer.for(str)
buf.set(:U8, 1, 111)
str
#=> "tost"
```
* It seems `to_str` always returns a string in `US-ASCII` encoding, I am not sure it is the right behavior. For a low-level buffer, I'd rather expect `ASCII-8BIT` (what file read in binary mode produces in Ruby).
**Inconsistencies and other problems**
* About `flags` as a creation parameter:
* `::new`: it seems that the only acceptable value is `MAPPED`; of others, `INTERNAL` is the default, `EXTERNAL`, `IMMUTABLE` and `LOCKED raise on creation;
* `::for`: no flags accepted (though it seems that a method to make such buffer immutable might be of some utility?)
* `::map`: `INTERNAL`, `LOCKED`, `EXTERNAL` are ignored, `MAPPED` is the default; seems like the only usage of the parameter is _not_ to pass `IMMUTABLE` which is the default: and for that, you'll need to also pass `size` and `offset`.
* It seems, that **we shouldn't probably expose "flags" concept to the user** at all, and instead make APIs like `::new(size, mapped: false)` and `::map(io, ..., immutable: true)`?
* The buffer raises `RuntimeError` as the only type of the error. I believe that in some places, it should be other standard errors (for example, "Offset + length out of bounds" could be at least `ArumentError`?..), and in other occurrences, special error classes: for example, "Buffer is locked" would probably be helpful to have as some `IO::Buffer::LockedError`
* Current `#inspect` is kinda nice and useful, but it also might be really annoying when used by Ruby itself. Try `IO::Buffer.map(File.open("big file")).unexisting_method`, and you'll get REALLY long error message.