Fixed incorrect behaviour on str impl.

[Frostie314159]

The default TryFromCtx impl for &str succeeds, even if a null byte is missing. This caused the unexpected behavior in #95. I'm unsure, if this is intended behavior, but while developing the previously referenced PR it was certainly unexpected.

[Frostie314159]

This PR replaces the take_while(...).count() approach, with Iterator::position.

[m4b]

while this isn't a breaking change, it's a pretty big semantic change to str parsing that i/we need to understand in detail; in particular, i need to understand if this somehow breaks goblin (which uses scroll's str parsing) somehow. otherwise i think it looks ok, though i haven't really reviewed in depth yet.
regardless the empty strs that were coming up in the scroll test in your fixed size array pr that prompted this was very counterintuitive (and as we see, bytes using 0 delimiter parsing has the expected size of 2 (instead of 3))

[Frostie314159]

On the note of goblin, I created a RegEx for matching at least the calls to read, with mention str explicitly: [pg]read(_with)?::<&str>" Maybe it can help.

[m4b]

somewhat unrelated, i want to release a new scroll version, but i'm on the fence whether i should release as 0.12 or 0.11.1, since it updates MSRV and (imho) this should be considered a breaking change, as it can unexpectedly break peoples builds. However, 1.63 rustc is from August 11th, 2022, so 1.5 years ago, which is quite long.
Lastly, I think we should figure out whether your change here is correct, and include that, and then cut a release

[m4b]

Ok as i feared, trying out this patch in goblin causes several failures:

failures:

---- pe::section_table::tests::set_name_offset stdout ----
thread 'pe::section_table::tests::set_name_offset' panicked at 'called `Result::unwrap()` on an `Err` value: Scroll(BadInput { size: 0, msg: "No delimiter was found." })', src/pe/section_table.rs:311:46
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- strtab::as_vec_no_final_null stdout ----
thread 'strtab::as_vec_no_final_null' panicked at 'called `Result::unwrap()` on an `Err` value: Scroll(BadInput { size: 0, msg: "No delimiter was found." })', src/strtab.rs:210:74

---- strtab::as_vec_no_first_null_no_final_null stdout ----
thread 'strtab::as_vec_no_first_null_no_final_null' panicked at 'called `Result::unwrap()` on an `Err` value: Scroll(BadInput { size: 0, msg: "No delimiter was found." })', src/strtab.rs:218:72

---- strtab::parse_utf8 stdout ----
thread 'strtab::parse_utf8' panicked at 'assertion failed: match Strtab::new_preparsed(&[0x80, 0x80], b\'\\n\') {\n    Err(error::Error::Scroll(scroll::Error::BadInput {\n        size: 2, msg: \"invalid utf8\" })) => true,\n    _ => false,\n}', src/strtab.rs:242:5


failures:
    pe::section_table::tests::set_name_offset
    strtab::as_vec_no_final_null
    strtab::as_vec_no_first_null_no_final_null
    strtab::parse_utf8

will need to do more of investigation to see what's going on; it's possible the behavior of scroll here is directly because of goblin's use (they were developed in tandem originally), but I don't know.

[m4b]

I've decided to release scroll 0.12 without this patch; we can investigate the root causes to this later, but for now, happy new year!

[Frostie314159]

Regarding this, we should maybe reconsider the no deps policy, for scroll, at least in the case of memchar. We could make this an optional feature, it probably won't hurt the library, since memchr is used internally in std. Memchr will fall back to the exact routines, we use now and can provide quite a decent performance improvement.

[m4b]

friendly ping about the status of this? I'd like to see a proper fix here, but avoiding breakage, don't know if that has been resolved.

re adding deps, there would have to be really compelling reason to add any new deps to scroll, backed with a good amount of empirical evidence justifying its inclusion. even then, it would be hard decision. over the years, scroll + goblin's strict dep policy has served us well, and i'm quite happy with the situation, but again, always open to good argumentation to potentially change my mind on this position 🤷

[Frostie314159]

I don't think that adding memchr is strictly necessary. I looked at the failing tests, and some of them explicitly test for this behavior. The docs don't accurately explain what the different Ctx's do (which we should change) and we should add a Ctx, which explicitly implements this behavior like MaybeDelimiter.