Avoid creating references to invalid data

HadrienG2 commented 5 years ago

This implements the changes discussed in #17 . Areas in the code where references are still necessary are properly annotated in the code, along with a discussion of what language-level fixes are needed to fix the problem in a zero-cost way.

(Non-zero cost ways are also envisionable, of course, for example Vec<T> len could be dumped alongside the serialized data, but I fear that such unsatisfactory performance compromises go against the spirit of abomonation.)

I also moved all tests to assert_eq so that they produce more exhaustive error messages and engaged in a few formatting tweaks which make the code more readable in my subjective opinion. Since looks are subjective, feel free to disagree and request me to revert any part of that ;)

Fixes #17.

frankmcsherry commented 5 years ago

Thanks very much! Let me get some time to walk through this, to make sure I grok what is going on, but the intent sounds very solid.

HadrienG2 commented 4 years ago

Apologies for the big rebase, but it should ultimately be a net benefit: commits serve a clearer purpose (and can thus be reviewed one by one easily) and irrelevant changes were pushed out of this PR.

frankmcsherry commented 4 years ago

No worries. Sorry for the delay in the review. I'm moving house this week, and employment is moving offices at the same time.

HadrienG2 commented 4 years ago

By the way, this PR breaks the API of the Abomonation trait, so 1/a version bump is in order and 2/abomonation_derive will need an update. I can provide a PR for said update as soon as a matching version of abomonation is available on crates.io.

(FWIW, #25 also breaks the API, so you may want to batch both into a single release if you choose to merge them)

HadrienG2 commented 4 years ago

Ping @frankmcsherry, do you have more time to look into these PRs nowadays?

TimelyDataflow / abomonation

Avoid creating references to invalid data #22