-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fixed Exclusive Access to Memory bug #1489
Conversation
left.codingPath = codingPath + left.codingPath | ||
right.codingPath = codingPath + right.codingPath | ||
if var left = self.left, var right = self.right { | ||
left.codingPath = codingPath + self.left!.codingPath |
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.
we need to avoid !
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.
Yep but that is already safe because of conditional binding, if it is nil then the whole block is skipped....
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.
@SandorDobi you're totally right that it is safe in this case but we like to avoid it out of principle. @martinlasek will provide an alternative in 1, 2, 3 .. ;)
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.
One way to solve it without having to force unwrap could be:
var reason: String {
if let left = left, let right = right {
var mutableLeft = left, mutableRight = right
mutableLeft.codingPath = codingPath + left.codingPath
mutableRight.codingPath = codingPath + right.codingPath
return "\(mutableLeft.reason) and \(mutableRight.reason)"
} else if let left = left {
var mutableLeft = left
mutableLeft.codingPath = codingPath + left.codingPath
return mutableLeft.reason
} else if let right = right {
var mutableRight = right
mutableRight.codingPath = codingPath + right.codingPath
return mutableRight.reason
} else {
return ""
}
}
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.
Yep. Sure. I have a rule to avoid temporary variables where it is don't needed.
but understand if You have a strict "no !" policy.
So I'll change PR.
Sources/Validation/Validatable.swift
Outdated
return errors.map { error in | ||
var error = error | ||
error.codingPath = codingPath + error.codingPath | ||
return errors.map { err in |
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.
Maybe we can go a consistent way here with a prefix mutable
and do it in all the places as well without the bang. Like for this:
var reason: String {
return errors.map { error in
var mutableError = error
mutableError.codingPath = codingPath + error.codingPath
return mutableError.reason
}.joined(separator: ", ")
}
left.codingPath = codingPath + left.codingPath | ||
right.codingPath = codingPath + right.codingPath | ||
if var left = self.left, var right = self.right { | ||
left.codingPath = codingPath + self.left!.codingPath |
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.
One way to solve it without having to force unwrap could be:
var reason: String {
if let left = left, let right = right {
var mutableLeft = left, mutableRight = right
mutableLeft.codingPath = codingPath + left.codingPath
mutableRight.codingPath = codingPath + right.codingPath
return "\(mutableLeft.reason) and \(mutableRight.reason)"
} else if let left = left {
var mutableLeft = left
mutableLeft.codingPath = codingPath + left.codingPath
return mutableLeft.reason
} else if let right = right {
var mutableRight = right
mutableRight.codingPath = codingPath + right.codingPath
return mutableRight.reason
} else {
return ""
}
}
@@ -49,9 +49,9 @@ internal struct OrValidatorError: ValidationError { | |||
/// See ValidationError.reason | |||
var reason: String { | |||
var left = self.left | |||
left.codingPath = codingPath + left.codingPath | |||
left.codingPath = codingPath + self.left.codingPath |
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.
That approach feels nice 👍
…conditionals as requested
Sources/Validation/Validatable.swift
Outdated
var mutableError = error | ||
mutableError.codingPath = codingPath + error.codingPath | ||
return mutableError.reason | ||
}.joined(separator: ", ") |
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.
Attention indentation. 😊
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.
Done. Crappy Xcode allways do me some quirks
…conditionals as requested and identitation :D
syncing to upstream
The last swift snapshot (2018-02-08) start to enforce the Exclusive Access to Memory (SE-0176) this cause to fail to build vapor Validation sources.
This PR is fixing that.