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

[stdlib][SR-4818] Add overflow assignment operators #15144

Merged
merged 4 commits into from Mar 19, 2018

Conversation

Projects
None yet
8 participants
@rockbruno
Contributor

rockbruno commented Mar 10, 2018

This adds the overflow assignment operators (&+= &-= &*=) that were missing from integer types.

To match how other operators are written, I moved the regular overflow operator's implementation to the new assignment operators. I am unsure if this will have performance implications, so please let me know if this is a bad idea! I also removed the trailing white space from it's declaration since the other operators did not have one.

Resolves SR-4818.
Resolves: rdar://problem/32035586

@CodaFi

This comment has been minimized.

Collaborator

CodaFi commented Mar 10, 2018

This seems like it requires an evolution proposal.

@airspeedswift

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Mar 10, 2018

I wrote a pitch some days ago and my overall feeling from the comments was that their absence was merely a bug. But If you guys think it will be better if this takes the proposal route, I'll gladly arrange one

@xwu

Glad you've gone ahead and looked into making the change. Some minor comments. I do suspect we'll probably need a Swift Evolution proposal, but it shouldn't be controversial.

@@ -2740,8 +2797,18 @@ ${unsafeOperationComment(x.operator)}
${operatorComment('&' + x.operator, True)}
@_inlineable // FIXME(sil-serialize-all)
@_transparent
public static func &${x.operator} (lhs: Self, rhs: Self) -> Self {
return lhs.${x.name}ReportingOverflow(${callLabel}rhs).partialValue

This comment has been minimized.

@xwu

xwu Mar 10, 2018

Collaborator

These implementations can be left alone. No need to have the compiler do more work than is necessary to arrive at the same result.

@_transparent
public static func &${x.operator}=(lhs: inout Self, rhs: Self) {
let result = lhs.${x.name}ReportingOverflow(${callLabel}rhs).partialValue
lhs = result

This comment has been minimized.

@xwu

xwu Mar 10, 2018

Collaborator

This can be a one-liner; no need for the temporary result.

@@ -122,7 +122,7 @@ def all_integer_or_real_binary_operator_names():
def all_integer_assignment_operator_names():
return ['%=', '<<=', '>>=', '&=', '^=', '|=']
return ['%=', '<<=', '>>=', '&*=', '&=', '&+=', '&-=', '^=', '|=']

This comment has been minimized.

@xwu

xwu Mar 10, 2018

Collaborator

I don't think these changes are required here. For example, &<<= isn't present.

@@ -588,6 +588,63 @@ def assignmentOperatorComment(operator, fixedWidth):
/// - Parameters:
/// - lhs: The value to divide.
/// - rhs: The value to divide `lhs` by. `rhs` must not be zero.
""",
'&+': """\
/// Stores the sum of the two given values in the left-hand-size variable, discarding any overflow.

This comment has been minimized.

@xwu

xwu Mar 10, 2018

Collaborator

Please wrap to 80 columns and follow the style of the documentation above; that is, "Adds the two values and stores..."

'&+': """\
/// Stores the sum of the two given values in the left-hand-size variable, discarding any overflow.
///
/// The masking addition operator (`&+`) silently discards any overflow that

This comment has been minimized.

@xwu

xwu Mar 10, 2018

Collaborator

Please modify this text so that it talks about the assignment operator you're documenting.

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Mar 10, 2018

Very well then. I'll arrange a formal proposal soon.
Thanks guys!

@nuclearace

This comment has been minimized.

Contributor

nuclearace commented Mar 12, 2018

@rockbruno Per Ben Cohen this might not need a formal review. I would email the core team list to get a definite answer though.

@natecook1000

This comment has been minimized.

Member

natecook1000 commented Mar 15, 2018

Was going to ask that you add tests for these alongside the tests for the non-assignment masking operators, but can't find those anywhere. Maybe validation-test/stdlib/FixedPointArithmeticTraps.swift.gyb, since that's where we're testing the ...ReportingOverflow methods. @moiseev?

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Mar 15, 2018

@natecook1000 When I first implemented this I located operator tests in validation-test/stdlib/NumericDiagnostics.swift.gyb (which is why I had added the assignment operators in utils/SwiftIntTypes.py in the original commit), but as far as I could understand the boilerplate, it seemed to be more about integer types than the operators themselves which confused me a bit

If you guys point me in the right direction, I'll gladly write proper tests for them!

(By the way, I sent an e-mail to the core team asking about the review-needs for this feature.)

@airspeedswift

This comment has been minimized.

Member

airspeedswift commented Mar 15, 2018

Discussed this amongst the core team and the agreement is the omission of these was an oversight and there's no need to go through a review to add them. Thanks!

@moiseev

This comment has been minimized.

Member

moiseev commented Mar 16, 2018

@bruno-rocha-movile

This comment has been minimized.

Contributor

bruno-rocha-movile commented Mar 16, 2018

@moiseev That looks like a good place! But since it's my first time doing this, before I pushed anything I wanted to ask: What should be the test coverage for a feature like this?

Looking at the tests from this file, it seems like this could be written in two ways:

  • Through a single simple arithmetic test, like:
tests.test("Simple-Masked-Overflow-Arithmetic") {
  expectEqual(Int8.max &+ 1, Int8.min)
  var x = Int8.max
  x &+= 1
  expectEqual(x, Int8.min)
  //and so on for other operators
}
  • By multiple tests that bounds check each integer type, like it's already happening with the regular operators in this class:
tests.test("${Type}/Add/Overflow") {
  func f(_ x: ${Type}) -> ${Type} {
    return x + 1
  }
  expectCrashLater()
  _ = f(${Type}.max)
} //but with masked operators instead

What's usually expected?

@moiseev

This comment has been minimized.

Member

moiseev commented Mar 19, 2018

Something like this should be OK:

tests.test("${Type}/MaskingAdd") {
  expectEqual(${Type}.max &+ 1, ${Type}.min)
  do {
    var x = ${Type}.max
    x &+= 1
    expectEqual(x, ${Type}.min)
  }
}

The trick with an f function used in your second snippet is there to avoid compile time overflow check, as in, if you write Int.max + 1 it will just not compile, but we do want to make sure that it would fail at runtime, hence the extra function call.

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Mar 19, 2018

Thanks a lot @moiseev. I took the liberty of separating the operators into pairs of tests (non-assignment/assignment) to match the style of the previous tests in that file, but if that's not ideal just give me a heads up and I'll do them like in your example.

(There were no tests for *, so that one ended up in a single test.)

tests.test("${Type}/MaskingMultiply") {
var x = ${Type}.max
x &*= 2
expectEqual(x, ${Type}.max &* 2)

This comment has been minimized.

@moiseev

moiseev Mar 19, 2018

Member

Since these operators are defined in terms of one another, it makes the test ... not testing anything =)
How about something like expectEquals(${Type}.min, (${Type}.max / 2 + 1) &* 2)?

This comment has been minimized.

@bruno-rocha-movile

bruno-rocha-movile Mar 19, 2018

Contributor

Good catch, I couldn't think of a guaranteed single way to test multiplication overflow across all types 🙂

What about this?

tests.test("${Type}/Multiply/MaskingOverflow") {
  expectEqual((${Type}.max / 2 + 1) &* 2, ${Type}.min)
}

tests.test("${Type}/MultiplyInPlace/MaskingOverflow") {
  var x = ${Type}.max / 2 + 1
  x &*= 2
  expectEqual(x, ${Type}.min)
}

This comment has been minimized.

@moiseev

moiseev Mar 19, 2018

Member

in expectXXX functions, the expected value is the first parameter.

Sorry, I did not notice it in the first pass through the code. Can you please update the usages?

This comment has been minimized.

@bruno-rocha-movile
@moiseev

This comment has been minimized.

Member

moiseev commented Mar 19, 2018

@swift-ci Please smoke test

@moiseev moiseev merged commit d50f60c into apple:master Mar 19, 2018

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
@moiseev

This comment has been minimized.

Member

moiseev commented Mar 19, 2018

Thanks @rockbruno for your contribution!

@xwu

This comment has been minimized.

Collaborator

xwu commented Mar 19, 2018

This is fantastic. Glad it's finally done. Thanks @rockbruno!

@bruno-rocha-movile

This comment has been minimized.

Contributor

bruno-rocha-movile commented Mar 19, 2018

Awesome. Thanks a lot guys!

@rockbruno rockbruno deleted the rockbruno:overflow-assignment-2 branch Aug 31, 2018

@rockbruno rockbruno restored the rockbruno:overflow-assignment-2 branch Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment