Fix issue 13632: Enhancement to std.string.strip #6023

aravindavk · 2018-01-10T09:04:36Z

Added second argument similar to Python str.strip

Second argument accepts list of chars to strip and strips only
those chars.

Examples:

"xyzhello".stripLeft("xyz") == ""hello"
"helloxy ".stripRight("xy ") == "hello"
"xhellox".strip("x") == "hello"

Signed-off-by: Aravinda VK mail@aravindavk.in

dlang-bot · 2018-01-10T09:04:38Z

Thanks for your pull request, @aravindavk! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	13632	Second argument for std.string.strip

wilzbach · 2018-01-10T09:33:29Z

std/string.d

@@ -2840,15 +2841,17 @@ if (isForwardRange!Range && isSomeChar!(ElementEncodingType!Range) &&
        auto c = input.front;
        if (std.ascii.isASCII(c))
        {
-            if (!std.ascii.isWhite(c))
+            if ((chars == "" && !std.ascii.isWhite(c)) ||


is null is the more idiomatic D way to check for empty strings.

Thanks for review, I will update the patch to use null and refresh the patch.

is null is most definitely not the way to check for empty strings. That's how you check that a string is actually null. A null string is empty, but an empty string isn't necessarily null. The idiomatic way to check for empty is the empty function.

Yes, please don't use null to check for empty string, as it will introduce subtle bugs where empty strings with a non-null .ptr will not get recognized as empty. Either use .empty or do an explicit length check chars.length == 0.

jmdavis · 2018-01-10T10:07:58Z

std/string.d

@@ -2827,7 +2828,7 @@ if (isConvertibleToString!Range)
    See_Also:
        Generic stripping on ranges: $(REF _stripLeft, std, algorithm, mutation)
  +/
-auto stripLeft(Range)(Range input)
+auto stripLeft(Range)(Range input, string chars = "")


Do we really want to be adding this as an optional argument instead of an overload? This is going to make the common case slower, because it's going to have to keep checking for whether chars is empty or not. IMHO, it would make far more sense to either provide a second overload or to make this a variadic template so that the presence of the second argument can be checked at compile-time, and the extra runtime checks can be avoided.

Makes sense. I will work on changing this as overloaded function as you suggested. Thanks for the review

burner · 2018-01-12T10:42:59Z

I'm pretty sure that is not @jmdavis had in mind when he said overload.
This api is way to easy to use wrong.
Please, split it into two functions.

jmdavis · 2018-01-12T10:58:00Z

Yeah, this is way too error-prone. The fact that you're providing a second function argument should be what determines whether the normal version of strip is used or not, not whether you passed a template argument and passed a second function argument. The simplest way to deal with this is to just create two separate functions:

auto stripLeft(Range)(Range input)
if(...)
{...}

auto stripLeft(Range)(Range input, string chars)
if(...)
{...}

That way, the function signatures are clear, and the efficiency of the existing stripLeft is not affected by the new one.

aravindavk · 2018-01-15T07:04:12Z

Yeah, this is way too error-prone. The fact that you're providing a second function argument should be what determines whether the normal version of strip is used or not, not whether you passed a template argument and passed a second function argument. The simplest way to deal with this is to just create two separate functions:

Using second argument as compile time switch didn't worked since value should be known during compile time.

Without duplicating the function content I couldn't achieve two overloaded functions. Template argument is used in the intermediate function, both strip(Range) and strip(Range, chars) calls that function.

auto stripLeft(Range, bool useCustomChars)(Range input, string chars)

auto stripLeft(Range)(Range input)
if(...)
{
    return stripLeft(Range, false)(input)
}

auto stripLeft(Range)(Range input, string chars)
if(...)
{
     return stripLeft(Range, true)(input, chars)
}

First function is internal function, I have to explore how it can be made private. Other two functions are exposed which doesn't use template parameter.

Thanks.

jmdavis · 2018-01-15T07:48:11Z

Without duplicating the function content I couldn't achieve two overloaded functions.

Just create two completely separate functions. It's simpler. e.g.

auto stripLeft(R)(R range)
if(...)
{
    ...
}

auto stripLeft(R)(R range, string chars)
if(...)
{
    ...
}

wilzbach · 2018-01-15T07:53:52Z

Without duplicating the function content I couldn't achieve two overloaded functions.

To avoid duplication, you could use a templated stripLeftImpl:

auto stripLeft(R)(R range)
if(...)
{
    stripLeftImpl!(Yes.useChars)(range, chars);
}

auto stripLeft(R)(R range, string chars)
if(...)
{
    stripLeftImpl!(No.useChars)(range, chars);
}

private stripLeftImpl(Flag!"UseChars" useChars, Args...)(Args args)
{
   alias range = args[0];
   ... code ...
   static if (useChars)
   {
       ... // args[1]
   }
}

jmdavis · 2018-01-15T07:57:56Z

To avoid duplication, you could use a templated stripLeftImpl:

If you go that route, the flag is pointless. The number of arguments gives you the information you need. There is zero need to pass any kind of template argument indicating which version of the function to use.

jmdavis · 2018-01-15T08:36:15Z

Looking over these again, I definitely vote for separate functions. Each overload should be doing fundamentally different things, and honestly, the fact that you've just swapped out isWhite for a call to indexOf implies that you don't really understand what the implementations of stripLeft and stripRight are doing.

With the new overload, for stripLeft, it's going to need to decode each character and compare it against the characters in chars. There's no point in the checks for isASCII and all of that, not without getting into checking that all of the characters in chars are ASCII and then having different branches depending on that information.

And with stripRight, that big long set of if-elses has to do with looking for Unicode whitespace efficiently and has nothing to do with chars

Just create completely separate overloads for these versions of stripLeft and stripRight, and don't just copy the existing ones and tweak them. How would you write these two functions if you weren't concerned about the existing overloads? Write their implementations as if the current ones didn't exist, and we can see what that would look like. The way that you've tried to tweak the existing ones really doesn't make sense. All of the fancy stuff in there is to make dealing with checking for whitespace more efficient and doesn't belong in overloads that aren't stripping whitespace.

aravindavk · 2018-01-15T08:39:15Z

Thanks @jmdavis for detailed review, will work on the suggested changes and refresh the patch by today.

aravindavk · 2018-01-16T13:12:44Z

Updated the code as suggested. Please review.

aravindavk · 2018-01-16T13:18:58Z

Build is failing in CI, looks like unable to find libdparse,

+ make -f posix.mak -j2 publictests DUB=dub BUILD=debug
make -C ../druntime -f posix.mak MODEL=64 DMD=../dmd/generated/linux/debug/64/dmd OS=linux BUILD=debug
mkdir -p generated/linux/debug/64/publictests || exists generated/linux/debug/64/publictests
make[1]: Entering directory '/home/circleci/druntime'
touch generated/linux/debug/64/publictests/.directory
make[1]: Nothing to be done for 'target'.
make[1]: Leaving directory '/home/circleci/druntime'
DFLAGS="-conf= -I../druntime/import  -w -de -dip25 -m64 -fPIC -g -debug generated/linux/debug/64/libphobos2.a -defaultlib= -debuglib= -L-ldl" dub build --force --compiler=${PWD}/../dmd/generated/linux/debug/64/dmd --single ../tools/tests_extractor.d
Root package tests_extractor references unknown package libdparse
posix.mak:610: recipe for target 'generated/linux/debug/64/tests_extractor' failed
make: *** [generated/linux/debug/64/tests_extractor] Error 2
rm generated/linux/debug/64/publictests/.directory
Exited with code 2

quickfur · 2018-01-17T18:14:01Z

std/string.d

@@ -2870,10 +2900,24 @@ if (isForwardRange!Range && isSomeChar!(ElementEncodingType!Range) &&
    assert(stripLeft([paraSep] ~ "hello world" ~ paraSep) ==
           "hello world" ~ [paraSep]);

+    assert(stripLeft("     hello world     ", " ") ==


Please put the new unittests in a separate unittest block, and also, please move the new stripLeft overload after the original unittest block. This is so that the generated documentation will make sense.

In other words, what you currently have is this:

/** ... */ auto stripLeft(...) ... // original stripLeft() /** ... */ auto stripLeft(...) // new stripLeft /// unittest { ... // original test cases ... // new test cases }

What it should be is this:

/** ... */ auto stripLeft(...) ... // original stripLeft /// unittest { ... } // original test cases /** ... */ auto stripLeft(...) ... // new stripLeft /// unittest { ... } // new test cases

quickfur · 2018-01-17T18:18:31Z

std/string.d

@@ -2882,6 +2926,12 @@ if (isConvertibleToString!Range)
    return stripLeft!(StringTypeOf!Range)(str);
 }

+auto stripLeft(Range)(auto ref Range str, string chars)


These two overloads also need to have the right order. Here's what the order should be:

/** ... */ auto stripLeft(...) ... // original stripLeft /// unittest { ... } // original test cases /// ditto <--- add this comment auto stripLeft(Range)(auto ref Range str) if (isConvertibleToString!Range) ... /// unittest { ... } // more original test cases /** ... */ auto stripLeft(...) ... // new stripLeft /// unittest { ... } // new test cases /// ditto <--- add this comment auto stripLeft(Range)(auto ref Range str, string chars) if (isConvertibleToString!Range) ... /// unittest { ... } // more new test cases (if there are any specific to the 2nd overload)

quickfur · 2018-01-17T18:19:49Z

std/string.d

+    assert(stripRight("     hello world     ", " ") ==
+           "     hello world");
+    assert(stripRight("     hello worldxy     ", "xy ") ==
+           "     hello world");


Same goes for stripRight. The original overload + unittests should be left as-is in their original order, and the new stripRight should be added after the original unittest, and the new tests should follow, in their own unittest block.

quickfur · 2018-01-17T18:20:56Z

std/string.d

+{
+    return strip!(StringTypeOf!Range)(str, chars);
+}
+


Same goes for these overloads. Keep the original single-argument overloads grouped together with their respective unittest blocks, followed by the new 2-argument overloads and the new unittests.

aravindavk · 2018-01-18T10:08:43Z

@quickfur thanks for the review, I will make the suggested changes and refresh the patch.

aravindavk · 2018-01-18T11:47:44Z

@quickfur updated the patch as suggested. Please review

aravindavk · 2018-01-19T05:10:31Z

Hi Reviewers, Let me know if any more changes required to the patch. Thanks.

quickfur

Looks much better now. The one thing that still stands out is that we can relax the requirements on the incoming range a bit more.

quickfur · 2018-01-19T17:37:53Z

std/string.d

+  +/
+auto stripRight(Range)(Range str, string chars)
+if (isSomeString!Range ||
+    isRandomAccessRange!Range && hasLength!Range && hasSlicing!Range &&


Technically, we don't need to require a random access range here. It is enough to require a bidirectional range and use back and popBack to strip from the end, in a similar way to stripLeft. In that case, we also don't need to require hasLength and hasSlicing, though of course in the implementation we can take advantage of those attributes where available.

quickfur · 2018-01-19T17:39:16Z

std/string.d

+  +/
+auto strip(Range)(Range str, string chars)
+if (isSomeString!Range ||
+    isRandomAccessRange!Range && hasLength!Range && hasSlicing!Range &&


ditto, we can relax the constraints here to just isBidirectionalRange!Range, and that should be sufficient to implement this function.

aravindavk · 2018-01-22T09:54:36Z

@quickfur updated the patch to use isBidirectionalRange, please review.

jmdavis · 2018-01-26T14:47:10Z

Is there a way I can retrigger this test?

It triggers on new commits. So, just do a git commit --amend with no changes to the message, and then git push -f to your github repo, and it'll rerun everything.

wilzbach · 2018-01-26T14:49:11Z

Is there a way I can retrigger this test?

Only people with write permissions can login at Jenkins -> I just restarted Jenkins for you.
You could do it yourself by changing your commit sha:

git commit --amend
git push -f

Note that we currently have a couple of issues with Jenkins. The one you saw is dlang/ci#135
tl;dr: we are aware of the Jenkins failures and they won't block your PR.

aravindavk · 2018-01-26T14:51:25Z

Thanks a lot @wilzbach @jmdavis

quickfur · 2018-01-26T17:28:02Z

LGTM

ping @andralex Looks good now?

aravindavk · 2018-01-30T02:31:37Z

Thanks for all the review comments. Let me know if any change required.

@andralex please review the changes. (jenkins/pr-merge failure is not related to the patch #6023 (comment))

andralex

getting there

andralex · 2018-01-30T02:40:10Z

std/string.d

+    assert(stripLeft(" hello ", "") == " hello ");
+}
+
+auto stripLeft(Range, Char)(auto ref Range str, const(Char)[] chars)


Public and undocumented not good. Please merge constraints and bodies with the overload above.

I followed similar to existing stripLeft(Range)(auto ref Range str). Confused because none of the functions with auto ref input are documented in this file. Please clarify.

We pursue lifting the quality of code overall, and in the process of doing that it's possible that new code is subject to a higher bar than existing code. Two standards arose from experience:

All public symbols must be documented

Prefer fewer overloads

I understand this can be frustrating - please bear with us. Thanks!

No problem, happy to work on the changes, just needed a clarification. Thanks

andralex · 2018-01-30T02:40:39Z

std/string.d

+           "     hello world");
+}
+
+auto stripRight(Range, Char)(auto ref Range str, const(Char)[] chars)


please merge with the other overload

andralex · 2018-01-30T02:40:59Z

std/string.d

+    assert(strip(" hello ", "") == " hello ");
+}
+
+auto strip(Range, Char)(auto ref Range str, const(Char)[] chars)


please merge with the other oveload

aravindavk · 2018-01-30T03:37:48Z

@andralex merged the overloads as suggested. Please review.

aravindavk · 2018-01-30T06:22:05Z

ci/circleci is failing due to following error.

posix.mak:599: recipe for target 'std/traits.publictests' failed
make: *** [std/traits.publictests] Hangup
make: *** Deleting intermediate file 'generated/linux/debug/64/publictests/.directory'
Too long with no output (exceeded 10m0s)

Is this a known failure or do I need to do anything to fix this?

wilzbach · 2018-01-30T09:05:47Z

Known

wilzbach · 2018-01-30T18:22:12Z

Known

And fixed: #6099

(I restarted your build on CircleCi)

wilzbach · 2018-01-30T18:26:34Z

std/string.d

+    {
+        for (; !str.empty; str.popBack)
+        {
+            if (chars.indexOf(str.back) == -1)


Why don't you use foreach here?
In this case there's any performance penalty of this abstraction.
#6092 (comment)

Here we're iterating from the back, so we'd need foreach_reverse or retro if you wanted to use an abstraction.

Or we could write this as:

while (!str.empty && chars.indexOf(str.back) != -1) str.popBack;

which to me reads more intuitively as "while the range is not empty and the back is a character to the stripped, pop the back".

Shall I change this to use while as suggested?

Not important, but I like the for form. Kurz und gut.

Not important, but I like the for form. Kurz und gut.

Yes, I was referring to foreach_reverse:

foreach_reverse (s; str) if (chars.indexOf(s) == -1) break;

FWIW LDC optimizes foreach_reverse a lot better as it avoid auto-decoding:

https://godbolt.org/g/k9tr4S

_D7example3fooFAyaQdZv: sub rsp, 72 mov qword ptr [rsp + 64], rcx mov qword ptr [rsp + 56], rdx mov qword ptr [rsp + 40], rdi mov qword ptr [rsp + 48], rsi mov rcx, qword ptr [rsp + 64] mov rdx, qword ptr [rsp + 56] mov qword ptr [rsp + 24], rdx mov qword ptr [rsp + 32], rcx mov rcx, qword ptr [rsp + 24] mov qword ptr [rsp + 16], rcx .LBB0_1: mov rax, qword ptr [rsp + 16] mov rcx, rax sub rcx, 1 mov qword ptr [rsp + 16], rcx cmp rax, 0 je .LBB0_6 mov eax, 1 mov rcx, qword ptr [rsp + 16] mov rdx, qword ptr [rsp + 32] mov sil, byte ptr [rdx + rcx] mov byte ptr [rsp + 15], sil movzx esi, byte ptr [rsp + 15] mov rdx, qword ptr [rsp + 40] mov rcx, qword ptr [rsp + 48] mov edi, 1 mov dword ptr [rsp + 8], eax call _D3std6string__T7indexOfTAyaZQnFNaNbNiNfQpxwxEQBs8typecons__T4FlagVQBqa13_6361736553656e736974697665ZQBoZl@PLT cmp rax, -1 jne .LBB0_4 jmp .LBB0_6 .LBB0_4: jmp .LBB0_5 .LBB0_5: jmp .LBB0_1 .LBB0_6: add rsp, 72 ret

I hope you notice that it could inline back completely.

For comparison, the for:

_D7example3fooFAyaQdZv: push r15 push r14 push r12 push rbx sub rsp, 24 mov r14, rsi mov r12, rdi mov qword ptr [rsp + 8], rdx mov qword ptr [rsp + 16], rcx test rdx, rdx je .LBB0_9 mov rdi, rdx mov rsi, rcx call _D3std5range10primitives__T4backTyaZQjFNaNdNfAyaZw@PLT mov edi, 1 mov esi, eax mov rdx, r12 mov rcx, r14 call _D3std6string__T7indexOfTAyaZQnFNaNbNiNfQpxwxEQBs8typecons__T4FlagVQBqa13_6361736553656e736974697665ZQBoZl@PLT cmp rax, -1 je .LBB0_9 lea r15, [rsp + 8] .LBB0_3: mov rbx, qword ptr [rsp + 8] test rbx, rbx je .LBB0_6 mov rdi, rbx mov rsi, r15 call _D3std3utf__T10strideBackTAyaZQrFNaNfKQmmZk@PLT mov eax, eax sub rbx, rax cmp rbx, qword ptr [rsp + 8] ja .LBB0_5 mov qword ptr [rsp + 8], rbx test rbx, rbx je .LBB0_9 mov rsi, qword ptr [rsp + 16] mov rdi, rbx call _D3std5range10primitives__T4backTyaZQjFNaNdNfAyaZw@PLT mov edi, 1 mov esi, eax mov rdx, r12 mov rcx, r14 call _D3std6string__T7indexOfTAyaZQnFNaNbNiNfQpxwxEQBs8typecons__T4FlagVQBqa13_6361736553656e736974697665ZQBoZl@PLT cmp rax, -1 jne .LBB0_3 .LBB0_9: add rsp, 24 pop rbx pop r12 pop r14 pop r15 ret .LBB0_6: lea rsi, [rip + .L.str] lea rcx, [rip + .L.str.1] mov edi, 69 mov edx, 92 mov r8d, 2206 call _d_assert_msg@PLT .LBB0_5: lea rsi, [rip + .L.str.1] mov edi, 92 mov edx, 2207 call _d_arraybounds@PLT

If we would have perfect abstraction, I would have suggested str.retro.until!(s => chars.indexOf(s) > 0);, but unfortunately that's - even for the LLVM optimizer - too much.

edit: after thinking about this, it's is probably better here to do auto-decoding there because the user-input of chars could consist of utf-8 chars :/

wilzbach · 2018-01-30T18:27:02Z

std/string.d

+        return stripLeft!(StringTypeOf!Range)(input, chars);
+    else
+    {
+        for (; !input.empty; input.popFront)


Why don't you use foreach here?
In this case there's any performance penalty of this abstraction.
#6092 (comment)

I think the catch here is that we need to iterate from the back, not from the front. So we'll either have to use foreach_reverse (ick), or std.range.retro, which then would require passing the range in by reference, since we need to guarantee that the range is actually modified in order to return it to the user. So it'll end up being something ugly like this:

import std.range : retro; foreach (ch; (&input).retro) { ... }

which is equally ick.

Oh wait, what I said only applies to the stripRight variant. Here we're moving forwards, so there's no foreach_reverse or retro needed.

I'd say the for loop here is justified (using foreach would require passing the range by ref, just in case, so it still entails the ugly &range syntax).

Or perhaps a more self-documenting way to write this would be:

while (!input.empty && chars.indexOf(input.front) != -1) input.popFront;

which, at least to me, reads more intuitively as "while the range is not empty and the front element is a character to be stripped, pop the front element".

Well, it boils down to whether we want to do auto-decoding or not ... which after thinking about this is probably better here because the user-input of chars could consist of utf-8 chars.

aravindavk · 2018-01-31T00:34:21Z

And fixed: #6099 (I restarted your build on CircleCi)

Thanks @quickfur

andralex · 2018-01-31T02:43:27Z

std/string.d

@@ -3177,6 +3289,89 @@ if (isConvertibleToString!Range)
    });
 }

+/// Ditto
+auto strip(Range, Char)(Range str, const(Char)[] chars)


An additional overload would take a third argument:

auto strip(Range, Char)(Range str, const(Char)[] leftChars, const(Char)[] rightChars)

and would strip leftChars on the left and rightChars on the right.

Nice. I will add it

Added the new overload as suggested and added the respective tests.

wilzbach · 2018-01-31T03:31:48Z

std/string.d

+///
+@safe pure unittest
+{
+     assert(stripLeft("     hello world     ", " ") ==


Indentation

wilzbach · 2018-01-31T03:32:15Z

std/string.d

+    assert(stripLeft("xxxyy    hello world     ", "xy ") ==
+           "hello world     ");
+
+    import std.array : array;


This is a good sign that it should be a separate unittest:

} /// unittest {

Added second argument similar to Python `str.strip` Second argument accepts a string of characters to strip and strips only those characters. Examples: "xyzhello".stripLeft("xyz") == "hello" "helloxy ".stripRight("xy ") == "hello" "xhellox".strip("x") == "hello" Signed-off-by: Aravinda VK <mail@aravindavk.in>

aravindavk · 2018-01-31T15:44:10Z

@andralex @quickfur @wilzbach is this patch looking good now.

Thanks

quickfur · 2018-02-01T02:48:24Z

Alright, I think this is good to go. Any further fixes can be done in followup PRs. Merge ahoy!

aravindavk · 2018-02-01T04:19:39Z

Feeling very happy to see my first contribution to D language :)

Contribution experience was very nice, review comments were very valid and helped me to understand the language better. Happy to contribute more to D language in future. Thanks a lot

PetarKirov · 2018-02-01T07:24:00Z

Nice work @aravindavk, you made a solid contribution to the standard library!

wilzbach · 2018-02-01T07:36:05Z

Feeling very happy to see my first contribution to D language :)

BTW it's online on the changelog already:

https://dlang.org/changelog/pending.html#std-string-strip

Happy to contribute more to D language in future.

👍
I have sent out an invite to our Slack channel too (it's a good way to quickly ask questions) - you deserve d it!

Also, https://issues.dlang.org is open 24/7 😉

aravindavk requested review from burner and JackStouffer as code owners January 10, 2018 09:04

dlang-bot added the Enhancement label Jan 10, 2018

aravindavk force-pushed the string_strip_enhancement branch from 01f9e15 to 41f03f9 Compare January 10, 2018 09:17

wilzbach reviewed Jan 10, 2018

View reviewed changes

jmdavis reviewed Jan 10, 2018

View reviewed changes

aravindavk force-pushed the string_strip_enhancement branch 2 times, most recently from 71546f4 to e88e56d Compare January 12, 2018 08:02

dlang-bot added the Needs Work label Jan 12, 2018

aravindavk force-pushed the string_strip_enhancement branch from e88e56d to 2f2dfbc Compare January 16, 2018 13:07

dlang-bot removed the Needs Work label Jan 16, 2018

quickfur requested changes Jan 17, 2018

View reviewed changes

aravindavk force-pushed the string_strip_enhancement branch from 2f2dfbc to c67239f Compare January 18, 2018 11:45

quickfur reviewed Jan 19, 2018

View reviewed changes

aravindavk force-pushed the string_strip_enhancement branch from c67239f to 2bd8a7a Compare January 22, 2018 09:53

wilzbach reopened this Jan 26, 2018

andralex requested changes Jan 30, 2018

View reviewed changes

aravindavk force-pushed the string_strip_enhancement branch from 5515402 to 23acfee Compare January 30, 2018 03:35

aravindavk force-pushed the string_strip_enhancement branch 2 times, most recently from 714108a to 3d37afa Compare January 30, 2018 04:04

wilzbach reviewed Jan 30, 2018

View reviewed changes

andralex reviewed Jan 31, 2018

View reviewed changes

aravindavk force-pushed the string_strip_enhancement branch from 3d37afa to 947c8cc Compare January 31, 2018 03:11

wilzbach reviewed Jan 31, 2018

View reviewed changes

aravindavk force-pushed the string_strip_enhancement branch from 947c8cc to 2346990 Compare January 31, 2018 04:14

andralex approved these changes Feb 1, 2018

View reviewed changes

quickfur added the auto-merge label Feb 1, 2018

dlang-bot merged commit 8d93656 into dlang:master Feb 1, 2018

Fix issue 13632: Enhancement to std.string.strip #6023

Fix issue 13632: Enhancement to std.string.strip #6023

Conversation

aravindavk commented Jan 10, 2018

dlang-bot commented Jan 10, 2018

Bugzilla references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burner commented Jan 12, 2018

jmdavis commented Jan 12, 2018

aravindavk commented Jan 15, 2018

jmdavis commented Jan 15, 2018

wilzbach commented Jan 15, 2018 • edited

jmdavis commented Jan 15, 2018

jmdavis commented Jan 15, 2018

aravindavk commented Jan 15, 2018

aravindavk commented Jan 16, 2018

aravindavk commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindavk commented Jan 18, 2018

aravindavk commented Jan 18, 2018

aravindavk commented Jan 19, 2018

quickfur left a comment

Choose a reason for hiding this comment

quickfur Jan 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindavk commented Jan 22, 2018 • edited

jmdavis commented Jan 26, 2018

wilzbach commented Jan 26, 2018

aravindavk commented Jan 26, 2018

quickfur commented Jan 26, 2018 • edited

aravindavk commented Jan 30, 2018

andralex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindavk commented Jan 30, 2018

aravindavk commented Jan 30, 2018

wilzbach commented Jan 30, 2018

wilzbach commented Jan 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quickfur Jan 31, 2018 • edited

Choose a reason for hiding this comment

wilzbach Jan 31, 2018 • edited

Choose a reason for hiding this comment

aravindavk commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindavk commented Jan 31, 2018

quickfur commented Feb 1, 2018

aravindavk commented Feb 1, 2018

PetarKirov commented Feb 1, 2018

wilzbach commented Feb 1, 2018

wilzbach commented Jan 15, 2018 •

edited

quickfur Jan 19, 2018 •

edited

aravindavk commented Jan 22, 2018 •

edited

quickfur commented Jan 26, 2018 •

edited

wilzbach commented Jan 30, 2018 •

edited

wilzbach Jan 31, 2018 •

edited

quickfur Jan 31, 2018 •

edited

wilzbach Jan 31, 2018 •

edited

wilzbach Jan 31, 2018 •

edited