Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Remove dependency on
solidity-stringutils
#91base: main
Are you sure you want to change the base?
Remove dependency on
solidity-stringutils
#91Changes from 1 commit
32c7c5c
8c13526
6dbf358
d2dbf48
d2c8419
6baa0e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thing that can be defined only once as a library constant:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work on some older versions of Solidity 0.8.x which should be supported for previous versions of OpenZeppelin Contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do without the Vm:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it would be pure, without would allow all the previously view function to remain view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use
Bytes.slice
here because we want to support OpenZeppelin Contracts 4.x and 5.x, and Bytes isn't available in some older versions. We also can't just copy in theBytes.slice
code because older versions of Solidity don't supportmcopy
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reasoning as my other comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the use of
vm.contains
, similar to the suggestion incount()
. This allows this to bepure
, and reverted some other functions' mutability to view.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the contains really needed ?
I'm expecting
vm.split(subject, search)
to return a string[1] that contains everything when search is not present in subject.So
tokens.length
is 1, andtokens.length - 1
is 0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that neither this is not a correct implementation of count. It will not return the correct value when the instances overlap. For example,
count("aaa", "aa")
should return 2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
vm.split
. Changed function documentation to say that this is for non-overlapping occurrences, as we currently don't have a use case for counting overlaps.