Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SkipRunes, DropRunes #6

Merged
merged 10 commits into from
Nov 13, 2022
Merged

Add SkipRunes, DropRunes #6

merged 10 commits into from
Nov 13, 2022

Conversation

matthewsanetra
Copy link
Collaborator

Per discussion in #3, I've implemented SkipRunes, DropRunes.

I have also made the following changes:

  • Optimised DropString
    • Previously, I allocated a new byte buffer and then cast it to a string, which allocates again. It was possible to cast it using unsafe and avoid the extra allocation, but at the time I figured it wasn't worth it as . Since then I have found strings.Builder, which internally performs the unsafe conversion and saves us an allocation.
  • Removed append in Drop
    • We know the length of the slice we are returning, but we set the length to 0 in the make call. It was overwritten by append, but we can eliminate that by just making the slice with the final length at the beginning and copying the elements with copy. Should have no (negative) change in performance as all allocations in Go are zero'd anyways (so no internal optimisations done by append).
  • Clarify doc comments
  • Remove unnecessary allocation in Skip, SkipString
    • We previously allocated a zero value when we skip more than the number of elements in the slice, possibly dropping the underlying array of the slice passed in to the function. This is the intended behaviour of Drop, but not Skip, and so now we return a slice of the same underlying array, but with length 0, maintaining a reference to that underlying array.
  • Split rune functions into separate file
    • I believe that with the project size increasing we should start splitting up new functions into different files for easier development.
  • Restrict Skip,Drop,... to non-negative integers
    • I realised that we don't actually have defined behaviour for when a user passes in a negative integer, but instead of adding checks and documenting, I reasoned that it would just be best to restrict this via the type system. The reason we don't want to accept negative integers is because we already mentioned an Unskip method, and it is up to debate whether we would ever expose such functionality (re. idiomaticity)
  • Separate String functions into their own file
    • Same reason why Runes functions are in their own file.

Copy link
Owner

@thecsw thecsw left a comment

Choose a reason for hiding this comment

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

There are two comments about local size variable consistency and for my own understanding, how the rune logic works. Thanks

ops.go Show resolved Hide resolved
ops.go Outdated Show resolved Hide resolved
runes.go Show resolved Hide resolved
strings.go Show resolved Hide resolved
strings.go Outdated Show resolved Hide resolved
strings.go Outdated Show resolved Hide resolved
@thecsw
Copy link
Owner

thecsw commented Nov 13, 2022

@matthewsanetra Regarding point 6, when we take uint as an input parameter, it is aliased to uint32, I believe. To avoid any superfluous casting on the user's side, what do you think about using constraints.Unsigned, which includes all uint flavors?

@matthewsanetra
Copy link
Collaborator Author

what do you think about using constraints.Unsigned, which includes all uint flavors?

Very good idea! Changing now.

Copy link
Owner

@thecsw thecsw left a comment

Choose a reason for hiding this comment

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

Approved -- let's hope the tests trigger for this PR

@thecsw
Copy link
Owner

thecsw commented Nov 13, 2022

Woohoo! Tests passed. Merging.

@matthewsanetra thanks for the updates! Great stuff 🚀

@thecsw thecsw merged commit 07cada2 into thecsw:main Nov 13, 2022
@matthewsanetra matthewsanetra deleted the runes branch November 13, 2022 23:22
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.

2 participants