Skip to content

Implement arbitrary for CString#257

Closed
ThomasdenH wants to merge 1 commit into
BurntSushi:masterfrom
ThomasdenH:master
Closed

Implement arbitrary for CString#257
ThomasdenH wants to merge 1 commit into
BurntSushi:masterfrom
ThomasdenH:master

Conversation

@ThomasdenH
Copy link
Copy Markdown
Contributor

@ThomasdenH ThomasdenH commented Feb 6, 2020

Closes #165. This implementation builds a CString from either a String or a Vec<u8>. It shrinks using the VecShrinker, everywhere making sure no null characters end up in the CString.

To prevent using as many allocations, it is built using an iterator, which is then filtered directly. I can change that if it's undesirable.

@ThomasdenH
Copy link
Copy Markdown
Contributor Author

Is this PR acceptable? Should I change anything?

@maxbla
Copy link
Copy Markdown
Contributor

maxbla commented Jun 6, 2020

@BurntSushi seems to keep pretty busy. He maintains quite a few libraries (just look at his github!). He takes a while to get around to reviewing PRs, but he often does eventually. Mentioning him with @ (as I already did) sometimes helps.

I can give you a comment in the meantime though.

I'm not convinced of the need for your ArbitraryIterator. You say it avoids excess allocations, but I don't understand how. You could do (0..).map(|_| Arbitrary::arbitrary(g)).filter(|&c| c != '\0').take(size).collect(), which I believe is equivalent to your CString code, both in final result and in number of allocations.

BurntSushi pushed a commit that referenced this pull request Dec 27, 2020
We add a little sophistication here for whether the CString is
completely valid UTF-8 or whether it's just an arbitrary mix of bytes.
(Excluding NUL of course.)

Fixes #165, Closes #257
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Arbitrary for CString

2 participants