skipOver should have a single argument with pred version by JackStouffer · Pull Request #5497 · dlang/phobos

JackStouffer · 2017-06-19T16:47:34Z

…e argument with pred version

dlang-bot · 2017-06-19T16:47:37Z

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close	Bugzilla	Description
✓	17525	std.algorithm.searching.skipOver should have a single argument with pred version

CyberShadow · 2017-06-19T23:13:41Z

Hmm, I guess it is a bit weird that this will skip things in a loop. Meaning, s.skipOver(" ") and s.skipOver!isWhite are going to do different things.

If we decide to keep the inconsistency, then perhaps returning size_t would be more fitting.

This function as it is certainly seems more useful, though. What do you think of naming it skipWhile? If we decide we will want a skipOver with an alias that skips at most one element, skipWhile could be implemented on top of it.

wilzbach · 2017-06-20T02:36:03Z

std/algorithm/searching.d

+ * Returns:
+ *     `true` if elements were skipped over, `false` otherwise.
+ */
+bool skipOver(alias pred, R)(ref R r) if (ifTestable!(typeof(r.front), unaryFun!pred))


A) countUntil

import std.algorithm.iteration : filter; import std.range; return (&r).refRange.chain.countUntil!(x => !unaryFun!pred(x)) > 0;

B) find

import std.range; return (&r).refRange.enumerate.find!(x => !unaryFun!pred(x.value)).front.index > 0;

How do you mean that?

How do you mean that?

It was just an FYI on how this functionality currently can be achieved. Of course, both ways are a bit clunky and won't be as fast as this implementation.

@andralex If we could get find to be partially instantiatable with just the predicate then that solves a bunch of problems at once, without introducing new symbols. Unfortunately actually doing it is tricky - see comments below.

@CyberShadow I think making find instantiable with the predicate would help a lot of cases. That doesn't necesarily compete with skipOver which is useful for parsing.

wilzbach · 2017-06-20T02:45:36Z

This function as it is certainly seems more useful, though.

Yes, I often use a combination of the "alternatives" I listed.

What do you think of naming it skipWhile? If we decide we will want a skipOver with an alias that skips at most one element, skipWhile could be implemented on top of it.

N.B.: skipWhile = until, dropWhile = find

There's a reason why I made this PR: #4985
Names in Phobos are sometimes confusing and make it hard to find the needed function
See also:

tl;dr: no new symbols just for negation (unfortunately)

`findSkip`

findSkip in contrast to find doesn't accept only a pred (it requires a needle)
Adding support for just pred would be a trivial alias with this extension ;-)

CyberShadow · 2017-06-22T05:49:49Z

Argh! find does not use the eponymous template pattern, which means it's not partially instantiatable with just a pred. You should be able to do this:

alias skipWhile(alias pred) = find!(not!(unaryFun!pred));

I'd say that we fix find if possible, and (especially since Andrei was previously explicitly against this), close this.

wilzbach · 2017-06-22T05:58:48Z

I'd say that we fix find if possible, and (especially since Andrei was previously explicitly against this), close this.

I would use the opportunity to fix findSkip as well ;-)
I can make a PR for these two later today.

CyberShadow · 2017-06-22T06:03:08Z

find's case is a bit complicated because of the two overloads.

CyberShadow · 2017-06-22T06:12:06Z

For this to work, find's overloads must be united into a single template block. We can't really put the unittests inside it, and there's no way to split it up, so this will mean putting the unittests for the one-argument and two-argument overloads together.

CyberShadow · 2017-06-22T06:13:54Z

That, or ignore the two-argument version of find... though, it would be nice if that worked as well, so you could do e.g. alias ifind = find!"toLower(a) == toLower(b)";.

CyberShadow · 2017-06-22T06:35:00Z

Here is a rough draft for a plan:

// Old definitions
static if (false)
{
	/// Two-argument
	Haystack find(alias pred="a==b", Haystack, Needle)(Haystack h, Needle n)
	{
		return h;
	}

	/// One-argument
	Haystack find(alias pred, Haystack)(Haystack h)
	{
		return h;
	}
}

/// New definitions
static if (true)
{
	/// Two-argument (no pred)
	Haystack find(Haystack, Needle)(Haystack h, Needle n)
	{
		return h;
	}

	template find(alias pred)
	{
		/// Two-argument (with pred)
		Haystack find(Haystack, Needle)(Haystack h, Needle n)
		{
			return h;
		}

		/// One-argument (with pred)
		Haystack find(Haystack)(Haystack h)
		{
			return h;
		}
	}

	/// Backwards compatibility for explicit
	/// find!(pred, Haystack, Needle) instantiations
	template find(alias pred, Haystack, Needle)
	{
		alias findPred = find!pred;
		alias find = findPred!(Haystack, Needle);
	}

	/// Backwards compatibility for explicit
	/// find!(pred, Haystack) instantiations
	template find(alias pred, Haystack)
	{
		alias findPred = find!pred;
		alias find = findPred!Haystack;
	}
}

/////////////////////////////////////

unittest
{
	// No pred
	assert(find("a", "b") == "a");

	// With pred
	assert(find!"a==b"("a", "b") == "a");
	assert(find!"a=='x'"("a") == "a");

	// Explicit instantiations (optional backwards compat)
	assert(find!("a==b", string, string)("a", "b") == "a");
	assert(find!("a=='x'", string)("a") == "a");

	// Partial instantiation (desired)
	alias find1 = find!"a=='x'";
	assert(find1("a") == "a");
	
	alias find2 = find!"a==b";
	assert(find2("a", "b") == "a");
}

We'll just need to move the tests for the with-predicate two-argument overload together with the with-predicate one-argument overload.

wilzbach · 2017-06-22T06:41:50Z

Here is a rough draft for a plan:

Sounds like you volunteered to do it yourself ;-)

CyberShadow · 2017-06-22T07:01:47Z

Argh. There's like 8 overloads!

ghost · 2017-06-23T01:01:03Z

std/algorithm/searching.d

+ *     `true` if elements were skipped over, `false` otherwise.
+ */
+bool skipOver(alias pred, R)(ref R r) if (ifTestable!(typeof(r.front), unaryFun!pred))
+{


Stop creating AST nodes for something that's done automatically (i.e default initialization).

That is bad advice.

I can't find a quote at the moment, but Walter has been always saying that default initialization is not meant to provide useful default values, but to make using un-initialized variables deterministic and, wherever possible, an error. When possible, the default value of a type is an invalid one (NaN for floating-point types and '\xFF' for chars, which is an invalid UTF-8 code unit); the reason why most other types are initialized to zero is that they do not have an "invalid" value.

So, do initialize your variables explicitly (when you feel it makes sense to) and do not rely on their default value.

What you didn't know is that the author of the PR is also the person who suggested to add a D-Scanner check for default initialization. My review comment couldn't be more legit.

Not relevant. The pull request is for Phobos, not some personal repository of Jack's.

andralex · 2017-06-29T23:46:07Z

std/algorithm/searching.d

+ *     r = the range to skip over
+ * Returns:
+ *     `true` if elements were skipped over, `false` otherwise.
+ */


Nice. Please add but use /// ditto for the documentation and consolidate with the existing doc.

RazvanN7 · 2017-07-05T09:38:38Z

Next time please add "Fix Issue $Number" so that the dlang bot can associate your PR with the bug fix. That way it can be found at a search through PRs. I couldn't find this PR so I opened another one with almost the same fix, as it can be seen above.

I think that this PR (or mine) should be merged and if consensus is reached with find, then we can modify skipOver.

My implementation avoids extra initializations and assignments by using a do while. I suggest you take a look before merging this.

CyberShadow · 2017-07-05T11:43:38Z

@andralex What do you think of fixing find so it's partially intantiatable with just the predicate (see above)? That would make this PR unnecessary or replaceable with a one-line alias.

andralex

The other implementation is more efficient and uses ///ditto. Shall we close this?

andralex · 2017-07-07T02:17:20Z

std/algorithm/searching.d

+ * Returns:
+ *     `true` if elements were skipped over, `false` otherwise.
+ */
+bool skipOver(alias pred, R)(ref R r) if (ifTestable!(typeof(r.front), unaryFun!pred))


@CyberShadow I think making find instantiable with the predicate would help a lot of cases. That doesn't necesarily compete with skipOver which is useful for parsing.

CyberShadow · 2017-07-07T04:11:31Z

I think making find instantiable with the predicate would help a lot of cases.

Agreed, but actually doing so without breaking code seems tricky (though still possible). I was hoping you might have some insights.

That doesn't necesarily compete with skipOver which is useful for parsing.

Oops, right, I was thinking of skipWhile above which returns a new range (like find) instead of mutating its ref argument (like skipOver).

wilzbach · 2017-07-07T22:31:51Z

Argh. There's like 8 overloads!

Yeah we first need to reduce them to be able to go ahead.
A first step: #5575 (more to follow)

Fix Issue 17525: std.algorithm.searching.skipOver should have a singl…

899ae55

…e argument with pred version

JackStouffer added the Severity:Enhancement label Jun 19, 2017

dlang-bot added the Severity:Bug Fix label Jun 19, 2017

CyberShadow requested a review from andralex June 19, 2017 23:13

wilzbach reviewed Jun 20, 2017

View reviewed changes

CyberShadow added the @andralex label Jun 22, 2017

ghost reviewed Jun 23, 2017

View reviewed changes

This was referenced Jun 23, 2017

[WIP] remove use of useless initializers #5508

Closed

bitfields2 #5490

Closed

JackStouffer removed the Severity:Bug Fix label Jun 26, 2017

andralex requested changes Jun 29, 2017

View reviewed changes

wilzbach mentioned this pull request Jul 5, 2017

Fix Issue 17525 - std.algorithm.searching.skipOver should have a single argument with pred version #5543

Merged

andralex reviewed Jul 7, 2017

View reviewed changes

andralex removed the @andralex label Jul 7, 2017

JackStouffer closed this Jul 7, 2017

wilzbach mentioned this pull request Jul 7, 2017

Unify needle overloads of std.algorithm.searching.find #5575

Merged

This was referenced Jul 8, 2017

[RFC] Make std.algorithm.searching.skipOver an eponymous template to allow partial instantiation #5576

Merged

Provide pred-only overload to std.algorithm.searching.findSkip #5577

Merged

Uh oh!

Conversation

JackStouffer commented Jun 19, 2017

Uh oh!

dlang-bot commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

CyberShadow commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

A) countUntil

B) find

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jun 20, 2017

Uh oh!

CyberShadow commented Jun 22, 2017

Uh oh!

wilzbach commented Jun 22, 2017

Uh oh!

CyberShadow commented Jun 22, 2017

Uh oh!

CyberShadow commented Jun 22, 2017

Uh oh!

CyberShadow commented Jun 22, 2017

Uh oh!

CyberShadow commented Jun 22, 2017

Uh oh!

wilzbach commented Jun 22, 2017

Uh oh!

CyberShadow commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jul 5, 2017

Uh oh!

CyberShadow commented Jul 5, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jul 7, 2017

Uh oh!

wilzbach commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dlang-bot commented Jun 19, 2017 •

edited

Loading

A) `countUntil`

B) `find`

CyberShadow Jun 23, 2017 •

edited

Loading