Skip to content
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

Prefer CGFloat.pi to CGFloat(M_PI) #1198

Closed
ghost opened this issue Jan 16, 2017 · 8 comments
Closed

Prefer CGFloat.pi to CGFloat(M_PI) #1198

ghost opened this issue Jan 16, 2017 · 8 comments
Labels
enhancement Ideas for improvements of existing features and rules.

Comments

@ghost
Copy link

ghost commented Jan 16, 2017

Swift supports CGFloat.pi for a CGFloat version of π as opposed to a Double. It also appears that M_PI is being deprecated.

It would be good in the obvious cases where the developer wants a CGFloat version of π to detect and autocorrect to use the new Swift-y constant, avoid the typecast, and make the code more readable.

This may make sense as an extension of legacy_constant?

For reference:

@marcelofabri marcelofabri added the enhancement Ideas for improvements of existing features and rules. label Jan 16, 2017
@marcelofabri
Copy link
Collaborator

This may make sense as an extension of legacy_constant?

Makes sense for me!

@ghost
Copy link
Author

ghost commented Jan 16, 2017

Hmmm... this may be better off in a separate rule. 🤔

The CGFloat.pi (and Float.pi) are only available in Swift 3+.

Also CGFloat(M_PI) results in two identifiers from SourceKitten, not one. So it's a different matching pattern than all of the rest of the legacy constants.

@marcelofabri
Copy link
Collaborator

I think semantically it makes more sense to be the same rule, even if the implementation is a bit different.

You can use SwiftVersion.current to switch on the Swift version.

@stephentyrone
Copy link

It's worth noting that CGFloat(M_PI) is very subtly different from CGFloat.pi on platforms where CGFloat is an alias of Float. Float.pi is 0x1.921fb4p1, but Float(Double.pi) is 0x1.921fb6p1. This is deliberate; π rounded to the nearest Float rounds up, outside the interval [0,π], which sometimes results in subtle bugs. So this change is more likely to fix bugs than cause them, but it's still something to be aware of.

@ghost
Copy link
Author

ghost commented Jan 17, 2017

@marcelofabri Sounds good. I'll continue down my the path of my initial implementation. I should have a PR raised later today.

@raid5
Copy link

raid5 commented Jan 23, 2017

This appears to have introduced a bug where autocorrect is converting CGFloat(M_PI) to CGFloat.pi in Swift 2.3 code.

@marcelofabri
Copy link
Collaborator

@raid5 I couldn't reproduce it. Could you fill an issue with more details?

@raid5
Copy link

raid5 commented Jan 23, 2017

@marcelofabri, no problem. #1238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Development

No branches or pull requests

3 participants