[PATCH] Robust basic_filebuf::imbue

[David Krauss <potswa at mac dot com>]

On Sep 23, 2010, at 6:03 PM, Paolo Carlini wrote:

> 1- Imbue clean up.
> 2- General audit for __check_facet, we have very few uses. Is it really
> needed? Should be removed altogether? Is the basic_filebuf case really
> different?


It turns out that __check_facet is completely necessary. The Standard says that filebuf behaves as if it called use_facet every time, and that is the behavior that __check_facet emulates. Other use cases of __check_facet correspond to similar language in the Standard.

On the other hand, the implementation of imbue is not only redundant with new code, but broken. The current behavior is

1. Throw if reading or writing and current facet is invalid.
2. No-op if reading or writing and current facet is stateful.
3. If writing, flush and write unshift.
If reading
	4. If new facet is valid and performs conversion and old facet is no-conversion, seek back to position.
	5. If old facet performs conversion, truncate consumed part of conversion buffer.

This leaves a lot of holes.

1. It's possible to set an invalid facet under any conditions, but impossible to subsequently set a valid one.
2. This is a very rough approximation of 27.8.1.4/17. See previous discussion with Jerry Schwarz; the requirement is unnecessary. In any case, quiet no-op is not a good way to implement undefined behavior.
3. This is correct.
4. This was formerly correct, causing the remaining get area to be re-read and converted. However, the implementation uses seekoff(0,cur) which is now a no-op.
5. This does not account for the possibility that the new facet is no-conversion. If switching from UTF-8 to "C", for example, the conversion buffer ends up holding what should be the contents of the get area.

My implementation uses a different strategy, and eliminates seeks entirely, which improves compatibility with non-disk files. Basically, it takes the buffer bytes that are still needed and puts them where the new facet will find them.

1. If writing, flush and write unshift.
If reading
	2. Normalize putback area.
	3. Skip past any following unshift sequence by getting the next character, ignoring any resulting exception from a codecvt invalid character error. (This enables block encoding schemes.)
	4. Find the distance from the logical file position to the physical position. This is the number of bytes we need to save.
	5. If the old facet is no-conversion, move bytes from the get area. Otherwise, move from the conversion buffer.
	6. If the new facet is no-conversion, move bytes to the get area. Otherwise, move to the conversion buffer.
	7. If moving to the conversion buffer but it doesn't exist, create it.
	8. If moving to the get area but it's not big enough, let the conversion buffer be the new get area.
	9. Move the bytes.
	10. Free the conversion buffer if it's no longer needed. Otherwise, clear the get area so the conversion buffer is used immediately.
11. Always imbue the given facet; never no-op.

(This is more steps, but they're simpler, and moreover the resulting behavior is correct.)

This makes it more practical to use a locale without a valid codecvt. I took a liberty and allowed seekoff to work with such a locale installed. The Standard says to throw std::bad_cast, but a user might want to seek to a byte location before imbuing. This is the only way, for example, to read a 2-byte encoded segment that starts at an odd offset, if there isn't a noconv facet on hand, which is likely for a weird char_type.

Also, I changed _M_terminate_output to allocate codecvt::max_length bytes for the unshift sequence. This enables block encodings of size > 128. Also, it matches the implementation of regular output conversion in _M_convert_to_external. (By the way, that is an unbounded alloca in _M_convert_to_external; we really should fix that.) The included testcase is a block encoding but the block size is only a few bytes.

Finally, the negative return value from _M_get_ext_pos got annoying so I positivized it. Also fixed the return type; matching the "int" from codecvt::length was stupid. Return types don't affect anything in the linker, right?

Attachment: filebuf_imbue.clog
Description: Binary data

Attachment: filebuf_imbue.patch
Description: Binary data