Entry API

[pitaj]

Related #11

[udoprog]

Will do! Thank you!

[udoprog]

Very nice!

Style-wise and beyond the specific omments the only things I'd consider changing are:

• Moving the option-bucket crate into fixed-map-derive (as a pub(crate) module). This would mean changing the forbid(unsafe_code) into a deny(unsafe_code) like you taught me before :). But that's not that big of a deal.
• Since the feature is so far along not bother with the entry feature flag. Rust is already quite capable at removing things which are unused.

If you do want to make this addition more incremental I'm fine with it too. As long as it's polished up before an ultimate release I don't mind. And if you find yourself in the future lacking the time to do it I'd be more than willing to pick up the missing pieces.

Please move ahead!

[udoprog]

Hm, in terms of invariants, I don't believe there is any hazard with performing a regular overwrite here and avoid the extra unsafe (unless I'm missing something). Any perf uplift avoiding the drop should also be negligible since 1) the compiler might be able to statically determine that it's None and 2) checking the None discriminator is fairly cheap all things considering.

It might also be nice to test it like this:

let old = self.outer.replace(value);
debug_assert!(old.is_none(), "oops");

[pitaj]

Hm, in terms of invariants, I don't believe there is any hazard with performing a regular overwrite here and avoid the extra unsafe (unless I'm missing something).

Certainly, there's no hazard in treating a None as an Option.

Any perf uplift avoiding the drop should also be negligible since 1) the compiler might be able to statically determine that it's None and 2) checking the None discriminator is fairly cheap all things considering.

While that is true, I also don't really see value in removing the unsafe usage, given all of the unsafe usage in SomeBucket. If it was the difference between some unsafe and zero unsafe, that would be a different story.

While it may seem insignificant, the entry API exists specifically to allow the programmer to avoid hitting these kinds of unnecessary checks. I'd like to keep it the way it is.

[pitaj]

• Moving the option-bucket crate into fixed-map-derive (as a pub(crate) module). This would mean changing the forbid(unsafe_code) into a deny(unsafe_code) like you taught me before :). But that's not that big of a deal.
• Since the feature is so far along not bother with the entry feature flag. Rust is already quite capable at removing things which are unused.

1. option-bucket actually needs to be exposed by fixed-map in order for it to be accessible in derive(Key) in dependent crates, so it needs to be public somewhere (just pushed a fix for this)
2. AFAIK, you can't import a proc macro crate also as a library, so I don't think it can live in fixed-map-derive
3. I think segregating all of the unsafe code into OptionBucket and keeping fixed-map as forbid(unsafe) is a good policy
4. By keeping OptionBucket as a separate crate, it can be made optional for the entry feature. This allows users to choose whether they want to avoid unsafe entirely
5. I think other people could find option-bucket useful, so I wrote it and documented it with that in mind. That's why SomeBucket::new_unchecked and NoneBucket::new_unchecked are public.

[udoprog]

• option-bucket actually needs to be exposed by fixed-map in order for it to be accessible in derive(Key) in dependent crates, so it needs to be public somewhere (just pushed a fix for this)
• AFAIK, you can't import a proc macro crate also as a library, so I don't think it can live in fixed-map-derive

Yeah, this is fine. I wanted them in fixed_map.

• I think segregating all of the unsafe code into OptionBucket and keeping fixed-map as forbid(unsafe) is a good policy
• By keeping OptionBucket as a separate crate, it can be made optional for the entry feature. This allows users to choose whether they want to avoid unsafe entirely
• I think other people could find option-bucket useful, so I wrote it and documented it with that in mind. That's why SomeBucket::new_unchecked and NoneBucket::new_unchecked are public.

There's a few primary reasons I don't want a separate crate: It's harder to maintain and it adds a dependency. Both are costs to a project and a user that you don't want to impose without a good reason outweighing them.

The former means we'll have to manage and publish a separate crate option-bucket and deal with the interactions of a public API and versioning compatibilities. In particular we'll have to be mindful about semver and cargo interactions when changing and improving the (relatively young) API. Introducing breaking changes in the future would be harder in a stage of development where this is very probable to happen.

The latter means that users interested in knowing what this crate is doing (for whatever reason) has to review the contents of a separate crate to understand it. I'm not convinced the size of the dependency carries its weight as a separate dependency.

It's not clear to me whether these costs are warranted as it currently only used as an implementation detail in the entry API of this crate. If you in the future found another concrete use somewhere else I would encourage you to break it out into a separate crate though. Despite that it is still very nice that you documented the API and added examples which can act as tests - even if they're internal.

As for being able to separate unsafe_code behind a feature, I don't consider that as a desirable goal in itself. In my mind the entry feature will disappear once its complete and be merged into Storage. It is a safe audited abstraction for the collections provided by this crate. You've already achieved something good by wrapping up most of the unsafe use in nice abstractions. And we do get most of the maintenance benefits from #[deny(unsafe_code)] in that it throws up a red flag in case someone tries to add unsafe in additional modules.

So I want you to keep it an internal module (for this review). If you find uses for this abstraction outside of this crate in the future it's fine to move it then, but please find them first.

[pitaj]

Alright, I've moved option-bucket to a module. I've kept new and new_unchecked as public, because it really simplifies doctests.

As for being able to separate unsafe_code behind a feature, I don't consider that as a desirable goal in itself.

Many users will disagree, and think avoiding unsafe entirely is a feature in itself. I think it would be good to keep the entry feature for that purpose.

In my mind the entry feature will disappear once its complete and be merged into Storage.

I think keeping StorageEntry separate is really beneficial to code organization.

But all of that is debatable after an experimentation period.

[udoprog]

I appreciate it!

Looks good so I'll be merging soon once I get to a computer.

Continued:

I think part of it is that a feature flag isn't the right way to exclude unsafe. Because they are additive across all dependencies you don't actually control their activation. So a no-default-features = false is not sufficient for excluding unsafe use if it's introduced through features.

Note that for the people that do care about restricring unsafe use I'd encourage them to use a project such as cargo-geiger (and it's companion projects) to monitor any dependency changes.

[udoprog]

Thank you!