Skip to content

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jul 24, 2017

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Description
17679 SortedRange.contains should be deprecated in favor of the generic canFind

@wilzbach wilzbach changed the title Fix Issue 17679 - SortedRange.contains should be deprecated in favor … Fix Issue 17679 - SortedRange.contains should be deprecated in favor of the generic canFind Jul 24, 2017
assert(r1.release() == [ 64, 52, 42, 3, 2, 1 ]);
}

deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I duplicated the relevant unittest, s.t. we continue to test contains

$(RED Deprecated - please use $(REF canFind, std,algorithm.searching) instead).
*/
// @@@DEPRECATED_2018-02@@@
deprecated("Please use std.algorithm.searching : canFind instead.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmdavis - I hope I have done it correctly this time?
IIRC the stages were:

  • mark in documentation as deprecated
  • remove from documentation
  • remove entirely

@bubnenkoff
Copy link

Should canFind be aliased/renamed to contains? It's more general naming.

@andralex
Copy link
Member

The apparent duplication is intentional and useful. canFind means "can run linear find and find this value", whereas contains means "quickly rummage for this value" and must finish in logarithmic time.

@JackStouffer
Copy link
Contributor

@andralex But canFind specializes for SortedRange, so there's no speed benefit to using one over the other.

@andralex
Copy link
Member

That's an opportunistic optimization. Calling canFind means "I don't care whether this is linear, just find the thing". It's nice to have that use contains if available.

@wilzbach
Copy link
Contributor Author

@andralex But canFind specializes for SortedRange, so there's no speed benefit to using one over the other.

Yep copy/pasting from Bugzilla:

Since #4907 (December 2016), find takes advantage of Sortedness:

// If the haystack is a SortedRange we can use binary search to find the needle.

@andralex
Copy link
Member

@wilzbach the usefulness of the distinction is in generic code that wants to make sure fast searching is available

@wilzbach
Copy link
Contributor Author

the usefulness of the distinction is in generic code that wants to make sure fast searching is available

Isn't it the opposite for generic code? It should only care about the canFind operation.
Btw I just looked at the Phobos code and the only time a function cares about whether sth. is of type SortedRange is find.
Of course, there was #3534 which proposed to do more specializations like: with isSortedRange!(Range, pred), but we don't even have isSortedRange and afaict the purpose of the SortedRange was to do the optimizations in the function (e.g. canFind) a la:

static if(isSortedRange!(Range, pred)
 // faster path
else
 // normal path

and not at the call site:

static if (hasMember!(Range, "contains"))
    r.contains(42);
else
    r.canFind(42);

Anyway I don't care about this too strongly. I just found on the NG that people struggle with it and find it confusing.

@andralex
Copy link
Member

Offering functions of distinct complexity is within the doctrine of generic programming; there's long established practice of such in the STL, viz. member vs. non-member find. The same applies to DbI at least for as long as there's no systematic way of getting the complexity of an algorithm by means of introspection. My BigO library may challenge that notion in the future, but as things stand reflecting complexity in the name of functions is robust established practice.

The Achilles's heel of checking for SortedRange is that not only sorted ranges have fast contains - a variety of hashed containers come to mind. Any range with fast searching should implement contains. That information is vital and could make the difference between working and practically impossible as anyone who's joined two tables would know.

@JackStouffer
Copy link
Contributor

@wilzbach Since Andrei has a good argument in favor and there's been no activity on this, I'm closing this. Please reopen if you have a counter point.

@wilzbach wilzbach deleted the fix-17679 branch December 12, 2017 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants