str::not_utf8 condition is a sign that the from_bytes API needs massaging

[pnkfelix]

The current code for str::not_utf8 condition does this:

        cond.raise(fmt!("from_bytes: input is not UTF-8; first bad byte is %u",
                        first_bad_byte as uint))

It might be more reasonable for condition handlers to recover in this situation if they had a reference to the original byte vector, rather than just a string describing what was wrong.

(In one case, the byte vector being passed is a ~[u8], but in another it is &[u8]; in the latter, case, attempting to do this would run into #5370, unless we were willing to allocate a copy when raising the condition, which is probably not a good idea.)

(Update: blake2-ppc makes a reasonable suggestion in his comment below, though it involves a more substantial revision to the code. I've generalized the title of this ticket accordingly.)

[bluss]

std::str should provide a decoder function that scans the byte stream and raises conditions to handle invalid data, to offer the normal Skip | Truncate | Replace | ReplaceDefault | Fail resolutions.

[catamorphism]

I'm responsible for this code. I'm fine with changing the condition to carry a byte vector instead of a message.

[bluss]

Here is some test code to check the performance difference of conditions vs. statically specifying the recovery strategy (inserting replacement char). Conditions have a small cost when setting up to catch them, and a comparatively big cost if they fire, but otherwise it's fine.

https://gist.github.com/anonymous/904d9e4c90a1a199f452

by the way pnkfelix, the from_bytes_slice(&[u8]) -> &str API can not do any modifications, so while it could report the invalid sequences correctly, it can't recover.

[huonw]

Adding to that last comment, even &mut [u8] can't recover in general, and splicing replacement bytes into a ~[u8] is O(n), since the replacement character is multiple bytes.

[bluss]

The general &[u8] -> ~str decoder with recovery would allocate a new str from the start anyway (as in the benchmark), so any replacement can be spliced in.

For an iterator interface, arbitrary-length replacements sound much more complicated.

[huonw]

For an iterator interface, arbitrary-length replacements sound much more complicated.

I don't think it's too hard, i.e. the following would allow splicing an arbitrary number of bytes into a stream (clearly one can generalise to streams of any type, and one could easily restrict to Option or another enum if it was only ever necessary to splice a small number of items at a time (and this number is bounded)):

struct FooDecoder {
    splice: ~[u8]
    // ... other fields ...
}

impl Iterator<u8> for FooDecoder {
    fn next(&mut self) -> Option<u8> {
        if !self.splice.is_empty() {
            // left-over splices from the previous `next` call.
            Some(self.splice.pop())
        } else {
            // ... normal activities ...
            if need_to_splice { 
                // splicing [x,y,z], need to go on in reverse order to match the `pop`
                self.splice.push(z);
                self.splice.push(y);
                return Some(x);
            }
            // ... more normal activities ...
        }
    }
}

[pnkfelix]

Hmm. I think this is actually a duplicate of #4837; closing in favor of that.

[huonw]

Reopening because #4837 is two-bugs-in-one and this is more specific.

[pnkfelix]

see also #6164 for a more open-ended discussion of problems here.

[emberian]

Conditions are gone, but I think this is still relevant.

[pnkfelix]

@cmr well maybe at this point we're better off closing this ticket and letting the remaining API discussion continue in #9516

[emberian]

Yeah, closing.