Skip to content

bufferByCountAndTime#26

Merged
fsprojectsgit merged 2 commits intofsprojects:masterfrom
eulerfx:master
Jun 1, 2015
Merged

bufferByCountAndTime#26
fsprojectsgit merged 2 commits intofsprojects:masterfrom
eulerfx:master

Conversation

@eulerfx
Copy link
Copy Markdown
Contributor

@eulerfx eulerfx commented May 31, 2015

No description provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Could you also please add a more systematic test that covers a wider range of input sizes and buffer sizes? e.g. compare to the tests I've been adding recently which always have some kind of "for" loop at the outer to cover a range of scenarios. thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think FsCheck would make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, almost certainly would help, though I've not much experience in driving it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the rest of the code uses "while" loops, for better or worse. These use a Landin's-knot like trick to amortize the allocation that controls the loop. You may want to compare perf between these approaches at some point.

fsprojectsgit added a commit that referenced this pull request Jun 1, 2015
@fsprojectsgit fsprojectsgit merged commit 66d3ac0 into fsprojects:master Jun 1, 2015
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.

3 participants