Issue 13429: make sicmp and icmp @nogc nothrow. #4933
Conversation
| @@ -6968,18 +6968,22 @@ static assert(Grapheme.sizeof == size_t.sizeof*4); | |||
| $(LREF icmp) | |||
| $(REF cmp, std,algorithm,comparison) | |||
| +/ | |||
| int sicmp(S1, S2)(S1 str1, S2 str2) if (isSomeString!S1 && isSomeString!S2) | |||
| int sicmp(S1, S2)(S1 r1, S2 r2) @nogc nothrow | |||
There was a problem hiding this comment.
Shouldn't add @nogc nothrow -- someone might want to use this where S1 and S2 can throw / allocate.
| * By using $(REF byUTF, std,utf) and its aliases, GC allocations via auto-decoding | ||
| * and thrown exceptions can be avoided, making `icmp` `@safe @nogc nothrow pure`. | ||
| */ | ||
| @safe @nogc nothrow pure unittest |
There was a problem hiding this comment.
Please don't remove unittests without a good reason.
There was a problem hiding this comment.
Well the comment should probably have been the only thing removed then. As the above unittest is no @nogc nothrow.
There was a problem hiding this comment.
Not quite -- "foo" isn't the same as "foo".byDchar.
|
The main handy thing from this PR is that is makes |
|
Looks good. I'd update the description. |
| { | ||
| if (ridx == str2.length) | ||
| if(str2.empty) |
There was a problem hiding this comment.
style issue: please add a space before the (
| @@ -6968,18 +6968,22 @@ static assert(Grapheme.sizeof == size_t.sizeof*4); | |||
| $(LREF icmp) | |||
| $(REF cmp, std,algorithm,comparison) | |||
| +/ | |||
| int sicmp(S1, S2)(S1 str1, S2 str2) if (isSomeString!S1 && isSomeString!S2) | |||
| int sicmp(S1, S2)(S1 r1, S2 r2) | |||
| if (isForwardRange!S1 && isSomeChar!(ElementEncodingType!S1) | |||
There was a problem hiding this comment.
Why require forward ranges? save is never used in this function
|
Benchmarks please before merge (with LDC) |
|
Auto-merge toggled on |
No description provided.