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

Avoiding infinite loop when the search string is empty #646

Merged
merged 5 commits into from Jul 18, 2016

Conversation

Projects
None yet
3 participants
@lorenzo
Contributor

lorenzo commented Jun 13, 2016

The String.indexes function enters an infinite loop when doing String.indexes "" "Some string", this is because the i variable is no advanced on each step of the loop since it will always return 0.

I chose to return an empty list for this cases, as I think it is the most sensible thing to do IMO, but another valid interpretation is that the empty string is contained in all of the indexes of the string.

In case the second interpretation is preferred, I can gladly change it :)

Avoiding infinite loop when the search string is empty
The `String.indexes` function enters an infinite loop when doing  `String.indexes "" "Some string"`, this is because the `i` variable is no advanced on each step of the loop since it will always return 0.

I chose to return an empty list for this cases, as I think it is the most sensible thing to do IMO, but another valid interpretation is that the empty string is contained in all of the indexes of the string.

In case the second interpretation is preferred, I can gladly change it :)
@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Jul 15, 2016

Contributor

You should add a test to this PR.

Contributor

eeue56 commented Jul 15, 2016

You should add a test to this PR.

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Jul 16, 2016

Contributor

@eeue56 Done

Contributor

lorenzo commented Jul 16, 2016

@eeue56 Done

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 18, 2016

Member

Thanks, this change makes sense to me! I'm trying to sort out the contributors agreement stuff at the moment. Can you either ping the email you sent previously or fill this out and send it to me? It's looking like we'll be able to do something nicer soon, so I apologize for the inconvenience.

Member

evancz commented Jul 18, 2016

Thanks, this change makes sense to me! I'm trying to sort out the contributors agreement stuff at the moment. Can you either ping the email you sent previously or fill this out and send it to me? It's looking like we'll be able to do something nicer soon, so I apologize for the inconvenience.

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Jul 18, 2016

Contributor

@evancz Sent to your email address

Contributor

lorenzo commented Jul 18, 2016

@evancz Sent to your email address

Show outdated Hide outdated src/Native/String.js
@@ -192,7 +192,7 @@ function indexes(sub, str)
var subLen = sub.length;
var i = 0;
var is = [];
while ((i = str.indexOf(sub, i)) > -1)
while (subLen > 0 && (i = str.indexOf(sub, i)) > -1)

This comment has been minimized.

@evancz

evancz Jul 18, 2016

Member

Wouldn't it be faster to have the check for subLen > 0 outside the while loop? Seems like it only needs to happen once in an if, not on every loop.

@evancz

evancz Jul 18, 2016

Member

Wouldn't it be faster to have the check for subLen > 0 outside the while loop? Seems like it only needs to happen once in an if, not on every loop.

This comment has been minimized.

@lorenzo

lorenzo Jul 18, 2016

Contributor

Sure, I can do that :)

@lorenzo

lorenzo Jul 18, 2016

Contributor

Sure, I can do that :)

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Jul 18, 2016

Contributor

@evancz Implemented your suggestion

Contributor

lorenzo commented Jul 18, 2016

@evancz Implemented your suggestion

Show outdated Hide outdated src/Native/String.js
}

This comment has been minimized.

@evancz

evancz Jul 18, 2016

Member

Better, but it can be optimized a bit more :) If you move the if above allocating i and is you can just write something like this:

if (subLen > 0)
{
    return _elm_lang$core$Native_List.Nil;
}

Which is cool in two ways. First, it means i and is do not even get allocated. Second, and more importantly, it uses the early return style, which I try to use in any C-like language. As a reader, it means I can be sure, nothing past this point is relevant to this code / some invariant is definitely true past this point.

@evancz

evancz Jul 18, 2016

Member

Better, but it can be optimized a bit more :) If you move the if above allocating i and is you can just write something like this:

if (subLen > 0)
{
    return _elm_lang$core$Native_List.Nil;
}

Which is cool in two ways. First, it means i and is do not even get allocated. Second, and more importantly, it uses the early return style, which I try to use in any C-like language. As a reader, it means I can be sure, nothing past this point is relevant to this code / some invariant is definitely true past this point.

This comment has been minimized.

@eeue56

eeue56 Jul 18, 2016

Contributor

(typo: should be subLen < 1)

@eeue56

eeue56 Jul 18, 2016

Contributor

(typo: should be subLen < 1)

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Jul 18, 2016

Contributor

@evancz done

Contributor

lorenzo commented Jul 18, 2016

@evancz done

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 18, 2016

Member

Great, thank you!

Member

evancz commented Jul 18, 2016

Great, thank you!

@evancz evancz merged commit 2873fca into elm:master Jul 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lorenzo lorenzo deleted the lorenzo:patch-1 branch Jul 19, 2016

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