-
Notifications
You must be signed in to change notification settings - Fork 10.5k
stdlib: some small improvements for Dictionary and Set #4887
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
Changes from all commits
504361f
36ef0e6
8b11847
aecbd63
5922aa8
a006d3c
9d631eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2523,7 +2523,7 @@ final internal class _Native${Self}StorageImpl<${TypeParameters}> { | |
|
||
@_versioned | ||
internal var _capacity: Int { | ||
return _body.capacity | ||
return _assumeNonNegative(_body.capacity) | ||
} | ||
|
||
@_versioned | ||
|
@@ -2532,7 +2532,7 @@ final internal class _Native${Self}StorageImpl<${TypeParameters}> { | |
_body.count = newValue | ||
} | ||
get { | ||
return _body.count | ||
return _assumeNonNegative(_body.count) | ||
} | ||
} | ||
|
||
|
@@ -2703,7 +2703,7 @@ struct _Native${Self}Storage<${TypeParametersDecl}> : | |
@_versioned | ||
@inline(__always) | ||
internal func key(at i: Int) -> Key { | ||
_precondition(i >= 0 && i < capacity) | ||
_sanityCheck(i >= 0 && i < capacity) | ||
_sanityCheck(isInitializedEntry(at: i)) | ||
|
||
let res = (keys + i).pointee | ||
|
@@ -2713,7 +2713,7 @@ struct _Native${Self}Storage<${TypeParametersDecl}> : | |
|
||
@_versioned | ||
internal func isInitializedEntry(at i: Int) -> Bool { | ||
_precondition(i >= 0 && i < capacity) | ||
_sanityCheck(i >= 0 && i < capacity) | ||
return initializedEntries[i] | ||
} | ||
|
||
|
@@ -2747,7 +2747,7 @@ struct _Native${Self}Storage<${TypeParametersDecl}> : | |
} | ||
|
||
internal func setKey(_ key: Key, at i: Int) { | ||
_precondition(i >= 0 && i < capacity) | ||
_sanityCheck(i >= 0 && i < capacity) | ||
_sanityCheck(isInitializedEntry(at: i)) | ||
|
||
(keys + i).pointee = key | ||
|
@@ -2804,8 +2804,9 @@ struct _Native${Self}Storage<${TypeParametersDecl}> : | |
} | ||
|
||
@_versioned | ||
@inline(__always) // For performance reasons. | ||
internal func _bucket(_ k: Key) -> Int { | ||
return _squeezeHashValue(k.hashValue, 0..<capacity) | ||
return _squeezeHashValue(k.hashValue, capacity) | ||
} | ||
|
||
@_versioned | ||
|
@@ -2933,6 +2934,7 @@ struct _Native${Self}Storage<${TypeParametersDecl}> : | |
} | ||
|
||
internal func assertingGet(_ i: Index) -> SequenceElement { | ||
_precondition(i.offset >= 0 && i.offset < capacity) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this fixing an existing mem-safety bug, or just compensating for the removal of asserts elsewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's compensating for the removed precondition in isInitializedEntry (called in the next line) |
||
_precondition( | ||
isInitializedEntry(at: i.offset), | ||
"attempting to access ${Self} elements using an invalid Index") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// RUN: %target-swift-frontend -O -emit-ir -parse-as-library -primary-file %s | %FileCheck %s | ||
// REQUIRES: swift_stdlib_no_asserts,optimized_stdlib | ||
|
||
|
||
// A dictionary lookup should not contain any trap. Nothing can go wrong | ||
// from the user's perspective. | ||
|
||
// CHECK-NOT: llvm.trap | ||
|
||
public func testit(_ s: [Int : Int]) -> Int? { | ||
return s[27] | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,54 +72,20 @@ HashingTestSuite.test("_mixInt/GoldenValues") { | |
|
||
HashingTestSuite.test("_squeezeHashValue/Int") { | ||
// Check that the function can return values that cover the whole range. | ||
func checkRange(_ r: Range<Int>) { | ||
var results = [Int : Void]() | ||
for _ in 0..<(14 * (r.upperBound - r.lowerBound)) { | ||
func checkRange(_ r: Int) { | ||
var results = Set<Int>() | ||
for _ in 0..<(14 * r) { | ||
let v = _squeezeHashValue(randInt(), r) | ||
expectTrue(r ~= v) | ||
if results[v] == nil { | ||
results[v] = Void() | ||
} | ||
expectTrue(v < r) | ||
results.insert(v) | ||
} | ||
expectEqual(r.upperBound - r.lowerBound, results.count) | ||
expectEqual(r, results.count) | ||
} | ||
checkRange(Int.min..<(Int.min+10)) | ||
checkRange(0..<4) | ||
checkRange(0..<8) | ||
checkRange(-5..<5) | ||
checkRange((Int.max-10)..<(Int.max-1)) | ||
|
||
// Check that we can handle ranges that span more than `Int.max`. | ||
#if arch(i386) || arch(arm) | ||
expectEqual(-0x6e477d37, _squeezeHashValue(0, Int.min..<(Int.max - 1))) | ||
expectEqual(0x38a3ea26, _squeezeHashValue(2, Int.min..<(Int.max - 1))) | ||
#elseif arch(x86_64) || arch(arm64) || arch(powerpc64) || arch(powerpc64le) || arch(s390x) | ||
expectEqual(0x32b24f688dc4164d, _squeezeHashValue(0, Int.min..<(Int.max - 1))) | ||
expectEqual(-0x6d1cc14f97aa822, _squeezeHashValue(1, Int.min..<(Int.max - 1))) | ||
#else | ||
fatalError("unimplemented") | ||
#endif | ||
} | ||
|
||
HashingTestSuite.test("_squeezeHashValue/UInt") { | ||
// Check that the function can return values that cover the whole range. | ||
func checkRange(_ r: Range<UInt>) { | ||
var results = [UInt : Void]() | ||
let cardinality = r.upperBound - r.lowerBound | ||
for _ in 0..<(10*cardinality) { | ||
let v = _squeezeHashValue(randInt(), r) | ||
expectTrue(r ~= v) | ||
if results[v] == nil { | ||
results[v] = Void() | ||
} | ||
} | ||
expectEqual(Int(cardinality), results.count) | ||
} | ||
checkRange(0..<4) | ||
checkRange(0..<8) | ||
checkRange(0..<10) | ||
checkRange(10..<20) | ||
checkRange((UInt.max-10)..<(UInt.max-1)) | ||
checkRange(1) | ||
checkRange(2) | ||
checkRange(4) | ||
checkRange(8) | ||
checkRange(16) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well toss a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add it |
||
|
||
HashingTestSuite.test("String/hashValue/topBitsSet") { | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
If only there were some way to communicate this in the type system... 🤔
Which remaining assertions is this eliminating after you've moved all these checks to debug-only?
Note from my own digging in the compiler: this is lowered to value ranges on loads (fast), and not llvm.assume (slow). Good!
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 actually can't remember exactly, but there were some "< 0"-checks in llvm code.