CI coverage is good, running both rustfmt and clippy on every push,
regardless of branch. It does not seem to run specifically on pull requests,
which, I believe, leads to Github not checking the merge commit
specifically but instead just individual commits on the remote branch. Not
too consequential in any case, just slightly unorthodox.

Since 0.2.0 (the previous review) decoding has been added which seems to have
resulted in an additional internal dependency (bit_field) that has not yet
reached 1.0. However, it remained stable for the past year. It's not
immediately clear why a simple shift-and-mask would not be clean enough here,
the traits provided by the dependency seem much more relevant if they help
enable an interface or generic implementation—which is both not required
here.

The decode interface looks sound and decently usable. There is an interesting
difference in that its _with variant returns a count on success whereas the
encoding methods do not. Additionally, it's unclear how to efficiently and
properly react to an OverflowError because neither the number of
successfully consumed input nor the actually required output would be
available to the caller.

The encode interface is unchanged since 0.2.0. Thus I'm preserving the
previous review.

Contains unsafe code as a mechanism to eliminate bounds checks by asserting
properties with unreachable_unchecked. However, this appears to make proper
(and only proper) use of the unsafe assertion that strings are valid UTF-8
given by the standard library.

The unsafe code is documented and the reasoning behind it is explained. The
only minor flaw is that it does not properly explain that the validity
invariant used is being tested for. Since it does reference both utf-8 and
the byte length in its argument this should be straightforward to the reader
though.

A slightly cleaner way of testing the conditions might have been matching the
first byte with integer ranges, which would have eliminated the last bounds
check without an additional unreachable assertion.

No strong rating due to superficially unimportant dependency, and concerns
about the maturity of the interface (see above).

It remains unclear if an iterator based design instead of callbacks was
considered, potentially placing control flow in the hands of the caller. This
may be important for some cases where buffers need to be flushed while
efficiently preserving the decoding state.

Contains unsafe code as a mechanism to eliminate bounds checks by asserting
properties with unreachable_unchecked. However, this appears to make proper
(and only proper) use of the unsafe assertion that strings are valid UTF-8
given by the standard library.

The unsafe code is documented and the reasoning behind it is explained. The
only minor flaw is that it does not properly explain that the validity
invariant used is being tested for. Since it does reference both utf-8 and
the byte length in its argument this should be straightforward to the reader
though.

A slightly cleaner way of testing the conditions might have been matching the
first byte with integer ranges, which would have eliminated the last bounds
check without an additional unreachable assertion.

No strong rating since the crate is incomplete and the trust of maintainers
is not clearly established.

Verified by reading the entire crate and ensuring that it matched the utf-8 -> ucs2 transformation format.

• positives
  • definitely conforms to transformations, definitely doesn't panic. Definitely safe.
• negatives
  • has Error::BufferUnderflow and Error::InvalidData which are both unreachable due to the guarantees of &str.