Skip to content

Conversation

@patrickfreed
Copy link
Contributor

SWIFT-221

This PR changes the way integers are read from and written with Document.

On 64-bit systems:

  • BSON int32 will be read to Int32
  • Int32 will be written to BSON int32
  • BSON int64 will be read to Int
  • Int64 will be written to BSON int64
  • Int will be written to BSON int64

On 32-bit systems:

  • BSON int32 will be read to Int
  • Int32 will be written to BSON int32
  • BSON int64 will be read to Int64
  • Int64 will be written to BSON int64
  • Int will be written to BSON int32

This has the notable effect that Int will always be round tripped, though it will be in a size according to the cpu arch.

Also as part of this PR, the protocol BSONNumber was introduced. All numeric values that conform to BSONValue should conform to BSONNumber as well. It facilitates conversion to the numeric types that have native representations in BSON.

@patrickfreed patrickfreed marked this pull request as ready for review April 10, 2019 23:45
@patrickfreed patrickfreed requested review from kmahar and mbroadst April 10, 2019 23:45
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall, just some suggestions and questions.

Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! this was a very tricky API to think through, great job 👏

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great job!

@patrickfreed patrickfreed merged commit 456bb5b into master Apr 16, 2019
@patrickfreed patrickfreed deleted the SWIFT-221 branch May 7, 2019 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants