Skip to content

Conversation

@MaxDesiatov
Copy link
Contributor

This initializer is needed to fully implement StaticString.

Depends on #34.

@MaxDesiatov MaxDesiatov marked this pull request as draft February 19, 2022 11:22
@MaxDesiatov MaxDesiatov marked this pull request as ready for review February 19, 2022 20:47

@_transparent
init?(bitPattern: Int) {
guard bitPattern != 0 else { return nil }
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason to prefer guard here rather than if bitPattern == 0 { return nil }


public extension _Pointer {
@_transparent
init(_ from: OpaquePointer) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think being more explicit and marking the methods as public is better than just marking the extension as public

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Feb 19, 2022

Choose a reason for hiding this comment

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

I hope a lot of these changes can be applied by formatters and linters. Do you have any preference for specific once? I use SwiftLint and SwiftFormat locally, would it make sense to create a separate PR with configurations for those?

Copy link
Owner

Choose a reason for hiding this comment

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

I would say that swift-format would be better than SwiftLint, but I don't think that swift-format really can format the code in the style that I would prefer. If you want to add a configuration for swift-format in a separate PR, I am okay with that.

@MaxDesiatov MaxDesiatov requested a review from compnerd February 19, 2022 20:55
@compnerd compnerd merged commit 9009ec8 into compnerd:main Feb 19, 2022
@MaxDesiatov MaxDesiatov deleted the pointer-protocol branch February 21, 2022 10:12
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.

2 participants