uazu / qcell

Statically-checked alternatives to RefCell and RwLock
Apache License 2.0
356 stars 22 forks source link

Optional dependancy on exclusion-set for no_std TCell #45

Closed geeklint closed 1 year ago

geeklint commented 1 year ago

Adds optional no_std TCellOwner creation protected by exclusion-set. TCell and TCellOwner (the types) are available in no_std code; the following methods are now cfg-guarded:

TCellOwner::default - (std || exclusion-set) TCellOwner::new - (std || exclusion-set) TCellOwner::try_new - (std || exclusion-set) TCellOwner::wait_for_new - (std || (exclusion-set && std))

The behavior of the Drop impl for TCellOwner is now conditional on which features are enabled; I put the cfg on the method rather than having separate impls since it seemed more likely to catch mistakes, and avoid a situation where std::mem::needs_drop returns false for some downstream user when that's never actually possible.

Creating this initially as a draft PR since it seems likely you'll also want some adjustments to the testing infrastructure.

uazu commented 1 year ago

Clippy checks are failing for 'alloc' . I will have a look tomorrow.

geeklint commented 1 year ago

Oh, I bet it has to do with the uses in the tcell module, still trying to pull things in. I thought it would be simpler not to cfg guard those but they probably need it.

uazu commented 1 year ago

I've pushed a change to your branch which fixes stuff up for the testing/docs/etc. Also I switched some of the cfg lines around so that they were all of the same form. Also, the Cargo.toml change forces a MSRV of 1.60, so I thought I might as well formalize that.

For TCellOwner::wait_for_new the condition (std || (exclusion-set && std)) reduces to just std, but I guess you did it this way so that it is clearer to the person reading the docs, so that they realize that they can add std to the exclusion-set feature. So I left that as it was.

uazu commented 1 year ago

I can see my change on your branch, but it's not showing up here. Anyway, if this all looks okay to you then I'll merge the PR. If my change doesn't come through I'll add it afterwards.

I think for testing the exclusion-set feature, the existing tests should be okay. Or can you think of anything else that I should be testing in this case?

geeklint commented 1 year ago

I think I see the commit here, unless there was more than one.

All that sounds good/correct to me.

uazu commented 1 year ago

Thanks. I'll check over it all again before doing a release

uazu commented 1 year ago

Okay, it's released now in 0.5.4. Thanks for getting this sorted out.