-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Simpler InDocumentOrder implementation. #3017
Conversation
@@ -0,0 +1,91 @@ | |||
// Copyright (c) Microsoft. All rights reserved. | |||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going by this discussion: dotnet/coreclr#1329 (comment), you probably have right to put your initials after copyright and purge Microsoft, if you don't work for Microsoft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did start with the file that got deleted in this (git doesn't show that well unless there's a commit that only moves a file), so its definitely a derived work that started with something copyright Microsoft, and hence they are a copyright-holder on the derived work. The changes are pretty substantial if measured as a percentage of the file, so even when not listing all contributors (as is not done on this project) one could make an argument for adding me as a listed copyright holder on the file, but not for replacing MS entirely, as far as I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is precedent for adding yourself. I've done it in coreclr :)
System.Xml.Linq.Extensions.InDocumentOrder() uses a copy of the implementation of System.Linq.Enumerable.OrderBy(), to copy that working implementation without having a dependency on System.Linq. This includes unused complications; supporting key-selection but only ever using identity, part of the support for CreateOrderedEnumerable (but incomplete, and if it was it would have no real effect as the ordering is total), other comparers though only one is ever used, and a generalised buffering approach that isn't shared with any other code and so gains nothing in being generalised. This commit replaces that with a more narrowly focused implementation, providing just what is used by InDocumentOrder, resulting in simpler and tighter code. Obsoletes issue #3008 in that the case it is concerned with now results in an immediate return from early in the call to CompareDocumentOrder(), so the suggestion in #3008 would not improve performance further.
1e0cf2d
to
bf7b3de
Compare
@justinvp I'd thought of using Array.Sort, but figured that there must have been some reason OrderBy was favoured, so went with the path of just cutting down the implementation there to what was actually used. Having slept on it, I think the reason for OrderBy was simply that when one has the dependency on System.Linq, it's a simple one-liner to use. The only other advantage I can think of is that if OrderBy was ever improved then so would that be (e.g. the sort of thing I suggest in #2401) but that doesn't apply once the dependency is broken, so really Array.Sort is the way to go. |
if (count > 0) | ||
{ | ||
Array.Sort(items, 0, count, XNode.DocumentOrderComparer); | ||
for (int i = 0; i != count; ++i) yield return items[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be return items;
after the end of the if block? I'm wondering if it needs to be an iterator at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items.Length >= count
, so this could only be done if count == items.Length
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though thinking about it further, without more work that would move the ToArray work to be done during the initial call rather than during the first MoveNext. Ok, let's leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I considered that, and decided against it for those two reasons, and also that if we're avoiding returning objects from linq methods that implement IList
when it would be potentially uesful, we should probably also avoid doing so when it's mostly a shortcut.
LGTM. @krwq, any concerns? |
Simpler InDocumentOrder implementation.
…ering Simpler InDocumentOrder implementation. Commit migrated from dotnet/corefx@713e110
System.Xml.Linq.Extensions.InDocumentOrder() uses a copy of the implementation
of System.Linq.Enumerable.OrderBy(), to copy that working implementation
without having a dependency on System.Linq.
This includes unused complications; supporting key-selection but only ever
using identity, part of the support for CreateOrderedEnumerable (but
incomplete, and if it was it would have no real effect as the ordering is
total), other comparers though only one is ever used, and a generalised
buffering approach that isn't shared with any other code and so gains nothing
in being generalised.
This commit replaces that with a more narrowly focused implementation,
providing just what is used by InDocumentOrder, resulting in simpler and
tighter code.
Obsoletes issue #3008 in that the case it is concerned with now results in an
immediate return from early in the call to CompareDocumentOrder(), so the
suggestion in #3008 would not improve performance further.