Cleanup Reverse tests, and refactor First/Last.

[jamesqo]

Refactor First and FirstOrDefault and Last and LastOrDefault to share some code. Also, cleanup the tests for Reverse (previously, this PR contained changes related to that method).

cc @stephentoub @VSadov @JonHanna

[jamesqo]

I tried writing this as return found ? first : throw Error.NoElements() initially. But it looks like that feature of C# 7 isn't enabled yet 😢

[stephentoub]

I don't understand the value of this change / extra code. This is simply to optimize the patterns:

source.Reverse().First();
source.Reverse().Last();

? Why wouldn't the author instead just write the simpler and faster:

source.Last();
source.First();

?

[jamesqo]

@stephentoub The caller may not always have source cached in a local, so it may be more convenient to just get the first from Reverse() rather then reevaluating source. For example:

var items = cus.Where(c => c.Id == x).Reverse();

items.First(); // As opposed to calling cus.Last(c => c.Id == x)
items.Last(); // As opposed to calling cus.First(c => c.Id == x)

Another example is that source.Reverse() could have been passed to another method, where First or Last is called on it.

An additional reason for making these changes was that they were pretty trivial-- there are more red diffs than green in this PR in src/. And the benefits are pretty substantial; we are not just eliminating virtual method calls, we are also reducing allocations/big O complexity.

[stephentoub]

The caller may not always have source cached in a local

Then they could do ToArray or ToList. We should not be in the business of trying to optimize for every possible combination of operators, especially ones that are quite rare. Can you show me examples of real-world code where someone does a Reverse and then gets both the First and the Last element of it, and where it makes any difference to overall performance?

there are more red diffs than green in this PR

That's because you've combined two largely unrelated changes. You combined the First/FirstOrDefault implementations to avoid duplicating code, and then separately you added an IPartition implementation to Reverse.

[jamesqo]

Then they could do ToArray or ToList.

OK, you have convinced me; I additionally just did a grep search through Roslyn's src/ and most of the usages are foreach or ToArray. I'll take this change out for now + reopen later if it turns out to be desirable. The refactoring + test changes will stay in this PR.

[stephentoub]

The same consolidation can't be done for the overloads that take a predicate?

[jamesqo]

@stephentoub Good idea. I've done this for the predicate overloads too. There are now more than 170 deleted lines 🎉

[stephentoub]

Why are you removing all of these?

[jamesqo]

@stephentoub They do not seem to hit any extra codepaths. It's also unlikely we will specialize Reverse.Select or Reverse.Where since Reverse does not deal with selectors or predicates.

[stephentoub]

Why are you removing the nullable tests?

[jamesqo]

@stephentoub I can add them back.

[jamesqo]

Nevermind, actually: They can't be fed into the [Theory] because xUnit cannot do type inference for nullables. (When an int? is boxed it's an int and xUnit will try to interpret the int?[] as an IEnumerable<int>.) I don't think there's a special case for nullables within Reverse.

[hughbe]

I thought I fixed this in xunit; maybe we need to update xunit?

[stephentoub]

Why two separate selects rather than just:

.Select(c => new object[] { c.Select(i => i.ToString()), string.Empty })

?

[jamesqo]

I copied and pasted this from Last, which has a different local var name from LastOrDefault. That's why GitHub is showing diffs here.

[jamesqo]

Same here; I didn't actually write any of this logic, I copied it from Last.

[jamesqo]

I ran into a subtle difference between LastOrDefault and Last while making this refactoring. See the issue: https://github.com/dotnet/corefx/issues/14183 Not sure whether it should be >= or >. cc @JonHanna

[JonHanna]

It should be >=, (while First uses < as they need to have opposite behaviour on tie-breaking to match the behaviour of doing a stable sort followed by getting the last or first item respectively).

[jamesqo]

I have some other optimizations in mind related to Where that also require First / FirstOrDefault to be refactored, so I will fold them into this PR shortly. (see https://github.com/dotnet/corefx/issues/13696 for more info)

[jamesqo]

The aforementioned optimizations turned out to be a bit more complicated than I expected, so I may separate them into a new PR, actually.

[jamesqo]

@stephentoub I think this PR should be GTG (assuming you have no more comments).

[stephentoub]

Thanks, @jamesqo.