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

Just a few std.mem changes #944

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@BraedonWooding
Contributor

BraedonWooding commented Apr 22, 2018

Much smaller PR

  • Trim allows you to just trim from left/right side (or both)
    • Can you do bitwise in Zig? It seems to not like it? Do you have to cast and bitwise?
  • Index commands do allow you to not stop at the first but stop at the last one.
    • While we could index backwards in these cases it would obfuscate the code too much, not worth the loss in readability
  • endsWith added to line up with startsWith.
@andrewrk

please squash your changes

std/mem.zig Outdated
/// Remove values from the beginning and end of a slice.
pub fn trim(comptime T: type, slice: []const T, values_to_strip: []const T) []const T {
var begin: usize = 0;
pub fn trim(comptime T: type, slice: []const T, values_to_strip: []const T, side: Side) []const T {

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

this should be trim, trimLeft, and trimRight, instead of an enum parameter.

This comment has been minimized.

@BraedonWooding
std/mem.zig Outdated
}
pub fn indexOfScalarPos(comptime T: type, slice: []const T, start_index: usize, value: T) ?usize {
pub fn indexOfScalarPos(comptime T: type, slice: []const T, start_index: usize, value: T, highest: bool) ?usize {

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

this should be indexOfScalarPos and lastIndexOfScalarPos instead of a boolean parameter.

This comment has been minimized.

@BraedonWooding

BraedonWooding Apr 23, 2018

Contributor

Fair, will do

std/mem.zig Outdated
return i;
if (slice[i] == value) {
out = i;
if (!highest) break;

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

lastIndexOfScalarPos should use a better algorithm than this, searching from the end.

This comment has been minimized.

@BraedonWooding

BraedonWooding Apr 23, 2018

Contributor

Yes as I said in my comment I didn't want to remove readability for that speed bonus but if you want them as separate functions I'll implement the superior algorithm :).

std/mem.zig Outdated
}
pub fn indexOfAnyPos(comptime T: type, slice: []const T, start_index: usize, values: []const T) ?usize {
pub fn indexOfAnyPos(comptime T: type, slice: []const T, start_index: usize, values: []const T, highest: bool) ?usize {

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

indexOfAnyPos and lastIndexOfAnyPos without the bool.

std/mem.zig Outdated
return i;
if (slice[i] == value) {
out = i;
if (!highest) break;

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

lastIndexOfAnyPos should have a better algorithm than this. at worst searching from the end, at best reverse boyer-moore or something like that.

std/mem.zig Outdated
}
pub fn indexOf(comptime T: type, haystack: []const T, needle: []const T) ?usize {
return indexOfPos(T, haystack, 0, needle);
return indexOfPos(T, haystack, 0, needle, false);

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

see above comments

std/mem.zig Outdated
return i;
if (eql(T, haystack[i .. i + needle.len], needle)) {
out = i;
if (!highest) break;

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

see above comments

This comment has been minimized.

@BraedonWooding

BraedonWooding Apr 23, 2018

Contributor

Yep, I'll just reply once or twice as to not repeat myself. I'll clean this up :).

@@ -355,6 +382,10 @@ pub fn startsWith(comptime T: type, haystack: []const T, needle: []const T) bool
return if (needle.len > haystack.len) false else eql(T, haystack[0 .. needle.len], needle);
}
pub fn endsWith(comptime T: type, haystack: []const T, needle: []const T) bool {

This comment has been minimized.

@andrewrk

andrewrk Apr 23, 2018

Member

can you add a test for this?

This comment has been minimized.

@BraedonWooding

BraedonWooding Apr 23, 2018

Contributor

Sure :). I'll add a test for starts with too.

Implemented trimLeft/trimRight, lastIndexOf, lastScalarIndex, ..., en…
…dsWith, added tests for indexOf, implemented commands and starts/endswith

@BraedonWooding BraedonWooding force-pushed the BraedonWooding:MemChanges branch from 73fdd19 to 96d1e51 Apr 24, 2018

@BraedonWooding

This comment has been minimized.

Contributor

BraedonWooding commented Apr 24, 2018

@andrewrk Squashed commits and made requested changes. Also I think the fail is not due to anything I did? It says it is a timer assert failure despite there being no link? I'll merge in master and redo it.

== Edit ==
For some reason it is saying I've edited files due to the merge but those files haven't actually changed so I have to implement it as two commits... Git is just being odd, I'll give it a look tonight.

@BraedonWooding BraedonWooding force-pushed the BraedonWooding:MemChanges branch 2 times, most recently from cf3d168 to 5183b39 Apr 24, 2018

@andrewrk andrewrk closed this in 13076d5 Apr 25, 2018

@andrewrk

This comment has been minimized.

Member

andrewrk commented Apr 25, 2018

I fixed some bugs and improved the API, and avoided the merge commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment