Skip to content
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

Proposal: Ordered Collection Diffing #21845

Merged
merged 17 commits into from Mar 6, 2019

Conversation

@numist
Copy link
Member

commented Jan 14, 2019

This PR is an initial implementation of the Ordered Collection Diffing proposal, which was pitched on the forums in December.

@Azoy

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Nit: stdlib uses 2 space tabs

@DevAndArtist
Copy link
Contributor

left a comment

@numist several lines of this patch are too long. Stdlib uses 2 spaces tabs and has a maximum line width set to 80 characters.

Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
@lorentey
Copy link
Member

left a comment

I had suggested a number of small(ish) adjustments -- mostly to follow the stdlib coding guidelines:

  1. The fully qualified names of internal/private types (and members) must have at least one underscore-prefixed component. For initializers, we put the underscore in the label of the first parameter.
  2. All declarations must have an explicit access level in the stdlib. (This is often redundant, but it helps a lot with reviewing what gets into the public API/ABI.)
  3. When possible, we prefer to have meaningful names for generic type parameters, function parameters and local variables. (I think x, y etc. works fine in the core diffing implementation, though.)
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/CollectionDifference.swift Outdated
Show resolved Hide resolved stdlib/public/core/Diffing.swift Outdated
Show resolved Hide resolved stdlib/public/core/Diffing.swift Outdated
Show resolved Hide resolved stdlib/public/core/Diffing.swift Outdated
Show resolved Hide resolved stdlib/public/core/Diffing.swift Outdated
Show resolved Hide resolved stdlib/public/core/Diffing.swift Outdated

lorentey and others added some commits Feb 22, 2019

[stdlib] Update diffing code to match stdlib coding conventions
- Add an underscore to private/internal symbols
- Make access levels explicit, even when they’re implied from context
- Conform to max line length of 80 characters
- Conformances declared on separate extensions that implement them
- Arrange member declarations so that stored properties appear first
- Expand single-letter function and type parameter names where there is an obvious name that’s more descriptive
- RangeReplaceableCollection.fastApplicationEnumeration → CollectionDifference._fastEnumeratedApply
Wrap `<` to fit in 80
Suggested by @DevAndArtist

Co-Authored-By: numist <github@numist.net>

@numist numist changed the base branch from master to swift-5.1-branch Feb 26, 2019

numist added some commits Feb 26, 2019

Merge pull request #1 from lorentey/numist/diffing
Preparations for landing numist/swift
@DevAndArtist

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

I hope it won't get shipped with @available(macOS 9999, iOS 9999, tvOS 9999, watchOS 9999, *) by accident. 😅

public init?<Changes: Collection>(
_ changes: Changes
) where Changes.Element == Change {
if !CollectionDifference<ChangeElement>._validateChanges(changes) {

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

FWIW: there's an idiomatic way to spell if ! called guard. Some people prefer it for preconditions.

This comment has been minimized.

Copy link
@numist

numist Feb 27, 2019

Author Member

Oops, this actually used to be a guard but the code got refactored a number of times >_<

}

@available(macOS 9999, iOS 9999, tvOS 9999, watchOS 9999, *) // FIXME(availability-5.1)
extension CollectionDifference.Index { // Comparable

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

Can't the conformance be removed from the declaration above, and uncommented here?

@available(macOS 9999, iOS 9999, tvOS 9999, watchOS 9999, *) // FIXME(availability-5.1)
extension CollectionDifference {
fileprivate func _fastEnumeratedApply(

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

At some point only internal and public declarations were allowed in the stdlib. I wonder what happened to that rule. @lorentey, @milseman do you know?

This comment has been minimized.

Copy link
@milseman

milseman Feb 27, 2019

Contributor

I believe that private and fileprivate can work (but not as usableFromInline) in a world where we're not sil-serialize-all

This comment has been minimized.

Copy link
@lorentey

lorentey Feb 27, 2019

Member

We started to occasionally use private in the stdlib last year. It's still a bit unusual, and the fileprivate/private mess has annoying interactions with our rule for explicit access levels. But as far as I know, the technical issues we had before have all been resolved now.

func append(
into target: inout Self,
contentsOf source: Self,
from index: inout Self.Index, count: Int) {

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

The convention is to put a closing paren and the opening curly on the new line unindented after the parameter list.

enumeratedOriginals += origCount
enumeratedInserts += 1
}
assert(enumeratedOriginals <= self.count)

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

assert is rarely the right choice for the stdlib code. Consider using _precondition, _debugPrecondition, or _internalInvariant as described here.

}

fileprivate subscript(position: Index) -> Element {
precondition((startIndex..<endIndex).contains(position))

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

s/precondition/_precondition perhaps?

This comment has been minimized.

Copy link
@numist

numist Feb 27, 2019

Author Member

I think _internalInvariant is actually more appropriate here since this only provides early detection of a code defect that would fail further along regardless. I'll go through all the calls to assert and update as appropriate.

var (x, y) = (x, y)
let (n, m) = (a.endIndex, b.endIndex)

var v = _SearchState<Source.Index, Target.Index>(consuming: &pathStorage)

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

Vrbl nms R nt vry rdbl.

This comment has been minimized.

Copy link
@numist

numist Feb 27, 2019

Author Member

Sorry, too used to writing pure C—vowels are expensive!

For what it's worth, in this specific case the variable names are cribbed directly from the pseudocode in the Myers' paper (linked in the comments of this file) so they are canonical.

This comment has been minimized.

Copy link
@lorentey

lorentey Feb 27, 2019

Member

I think the terse names are justified here since the implementation follows the paper, and using the same notation makes the code easier to follow.


for (source, target, expected, line) in expectedChanges {
let actual = target.difference(from: source).inferringMoves()
expectEqual(actual, CollectionDifference(expected), "failed test at line \(line)")

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

StdlibUnittest expect functions take expected first and actual second.

let mine = "Is\nit\nreview\ntime\nalready?"

// Split the contents of the sources into lines
let baseLines = base.components(separatedBy: "\n")

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

Isn't it an NSString API? How is it even available here without import Foundation, I wonder.

This comment has been minimized.

Copy link
@milseman

milseman Feb 27, 2019

Contributor

Maybe through StdlibUnittest

This comment has been minimized.

Copy link
@numist

numist Feb 27, 2019

Author Member

I've only run the tests on Darwin, it seems likely that Foundation is getting imported somewhere. I'll make the test more literal.

let expectedChanges: [(
source: [String],
target: [String],
changes: [CollectionDifference<String>.Change],

This comment has been minimized.

Copy link
@moiseev

moiseev Feb 27, 2019

Contributor

Oh.. So we don't need availability checks if the APIs are marked as 9999?

This comment has been minimized.

Copy link
@lorentey

lorentey Feb 27, 2019

Member

Ah, good catch -- I haven't run the tests after the availability commit.

This comment has been minimized.

Copy link
@milseman

milseman Feb 27, 2019

Contributor

I still have to guard for it, but it will succeed at runtime

numist and others added some commits Feb 27, 2019

Merge pull request #2 from lorentey/numist/diffing
[test] Add missing runtime availability check around diffing tests
@lorentey

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Build failed
Swift Test OS X Platform
Git Sha - ed7cabb

@numist numist changed the base branch from swift-5.1-branch to master Mar 5, 2019

@numist

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Build failed
Swift Test OS X Platform
Git Sha - 4f783fb

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Build failed
Swift Test Linux Platform
Git Sha - 4f783fb

@Catfish-Man

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Build failed
Swift Test Linux Platform
Git Sha - fcf6550

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Build failed
Swift Test OS X Platform
Git Sha - fcf6550

@lorentey

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

The macOS failure in Swift Syntax was fixed in apple/swift-syntax#106.
Foundation's test_xpath failure on Linux also seems unrelated; it hasn't occurred in neighboring builds.

19:10:48 Test Case 'TestXMLDocument.test_xpath' started at 2019-03-06 01:10:47.995
19:10:48 
19:10:48 
19:10:48 0% tests passed, 1 tests failed out of 1
19:10:48 
19:10:48 Total Test time (real) =  21.82 sec
19:10:48 
19:10:48 The following tests FAILED:
19:10:48 	  1 - TestFoundation (SEGFAULT)
19:10:48 Errors while running CTest
19:10:48 FAILED: CMakeFiles/test.util 
@lorentey

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Third time the charm!

@swift-ci test

@Catfish-Man Catfish-Man merged commit 909868f into apple:master Mar 6, 2019

4 checks passed

Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details
@numist

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

@lorentey how did u do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.