Add Buf_write.printf (custom formatter)

[SGrondin]

After a discussion with @talex5 I rewrote #652 using a custom formatter and a mutable boolean instead of a symbolic formatter. It's shorter and more performant but it adds just a liiiitle bit more mutable state. I'm reminded of this famous xkcd.

[talex5]

Thanks - that's a lot simpler!

I'm not sure the formatter needs to be part of the writer at all. I pushed a commit which makes it entirely separate, so that each call to printf gets a formatter in the default state. I'm not sure whether that's an API improvement or not - it's not clear whether people will want to persist configuration changes between prints - but it's nice not having extra fields in Buf_write.t.

I wonder if we should only provide printf. The user can always do printf "%t" f to get access to the formatter if they want it, and it's easy enough for them to make their own custom formatter (with whatever flushing behaviour they prefer) for complex cases.

[SGrondin]

I took a few days to think about this.

1. I think the change from get_formatter to make_formatter is a substantial improvement.
2. I'm torn about recreating the formatter on every use of printf. You've mentioned that OCaml format strings are quite slow... but recreating the (quite large) formatter record on every call seems extreme just to save an 8-byte pointer in the writer record. I think it would be surprising behavior, too. I understand the user can make their own formatter, but then they also need to flush it themselves, etc, and while none of that added complexity is hard, I don't think any of it is necessary to put on the user. It's also potentially twice the number of values for the user to store or pass around.

Please let me know what you think of the commit I just pushed. I kept all of your changes, but I readded the formatter to the Buf_write record, this time only using a single field instead of 2, so 8 bytes instead of 16, plus it's initialized with a None so there's no extra allocations unless you use printf.

Thanks as always for your time and feedback, the resulting APIs we've developed together have so far been better than either one of our individual starting points.

[talex5]

With your commit, it fails on the second use with:

Fatal error: exception Sys_error("Buf_write.printf: invalid concurrent access")

It just needs to reset is_formatting when using the cached version. Now that it's stateful, it would be good for the unit-test to test using it twice.

[talex5]

Maybe we should call this Buf_write.format, in case we want to add a printf that does Printf.sprintf later.

[SGrondin]

Fixed and rebased.

Yeah, that was an embarrassing oversight on my part. 😅

Maybe we should call this Buf_write.format, in case we want to add a printf that does Printf.sprintf later.

I don't think I understand what Buf_write.sprintf would do. If sprintf means writing into a string, I don't see how it would be different from Format.sprintf itself?

[talex5]

Thanks!

I don't think I understand what Buf_write.sprintf would do. If sprintf means writing into a string, I don't see how it would be different from Format.sprintf itself?

It's just a bit faster if you don't need the extra formatting. Probably doesn't matter.