search

pchampin / mownstr

MownStr: Maybe Owned String
1 stars 2 forks source link

use Box::leak instead of forget #8

Closed pchampin closed 1 week ago

pchampin commented 1 month ago

attempt to address #5

pchampin commented 1 month ago

@mkatychev, could you check whether this solves the problem described in #5?

mkatychev commented 1 month ago

@pchampin this does indeed solve the problem. ideally I would like a unit tests that would cover this scenario to prevent regression.

The test_hash unit tests managed to reproduce the scenario best. On latest master, inserting mown1 and using println! on it seems to do the UB.

A CI miri test would also help avoid this in the future.

Addressing some of the clippy problems (such as the MownStr::from_str method name) for a new (non-hotfix) release would also be helpful.

Let me know if you'd like some help with the unit test/CI miri work.

pchampin commented 3 weeks ago

Let me know if you'd like some help with the unit test/CI miri work.

Yes, that would be very much appreciated. :+1:

pchampin commented 1 week ago

I merged this branch locally, but for some reason github does not see it as merged, and complains about conflicts with the 'main' branch. This is probably due to the fact that I merged 'main' in this PR at some point...

Closing it, but note that it was effectively merged.