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

Make precendence of basic math operators more explicit. #51

Open
davidbjames opened this issue Sep 12, 2019 · 6 comments
Open

Make precendence of basic math operators more explicit. #51

davidbjames opened this issue Sep 12, 2019 · 6 comments

Comments

@davidbjames
Copy link

This caused me some grief with recent update to Xcode 11 GM. Somehow, Swift (5.1) started re-ordering some of DynamicColor's math expressions to use + and - overloads I had created to handle mixing CGFloat and Int without the need to cast. This was possible because DynamicColor code and mine are in the same module. Strangely, this did not occur before Swift 5.1

The change in Swift's behavior caused major breakage in my app colors, because the math expressions became incorrect -- for example, let r = hueToRGB(m1: m1, m2: m2, h: h + 1 / 3) would compute h + 1 before / 3.

Notwithstanding the dumbness of my operator overloads (!), I think DynamicColor should make these expressions more explicit to shield future problems from occurring.

Using that same example, change:
let r = hueToRGB(m1: m1, m2: m2, h: h + 1 / 3)
to
let r = hueToRGB(m1: m1, m2: m2, h: h + (1.0 / 3.0))

And of course, other places where similar precedence problems may occur.

I would be happy to create the pull request for this as soon as I have a moment. Else, if someone else does I'd be happy to review it.

@yannickl
Copy link
Owner

Hey @davidbjames, I will update the project to fix that. :)

@davidbjames
Copy link
Author

Thanks for doing this @yannickl

I took at look at the commit and would make a small suggestion for improvement. There a few places where integers are used in expressions that returns floats. For example:
https://github.com/yannickl/DynamicColor/blob/master/Sources/HSL.swift#L112 (there may be other places like the Lab file for one..)

In my testing the presence of these integers was still causing the precedence (and my + overload) to misfire, even with the parenthesis. E.g. h + (1 / 3) was still being interpreted as (h + 1) / 3. This may be a Swift bug, but probably better to be safe than sorry.

@yannickl
Copy link
Owner

In my testing the presence of these integers was still causing the precedence (and my + overload) to misfire, even with the parenthesis. E.g. h + (1 / 3) was still being interpreted as (h + 1) / 3. This may be a Swift bug, but probably better to be safe than sorry.

This is very strange, how your operator overload can affect the parenthesis precedence?! Moreover here the type inference should treat these constants as CGFloat so how it can affect precedence?

I'm not following the Swift language evolution anymore so I don't know what is the root cause. :/

@davidbjames
Copy link
Author

When I was troubleshooting this problem in Xcode I tried your latest version first h + (1 / 3) and set a breakpoint in my overload and it hit with the backtrace coming from that line in DynamicColor. This also seemed VERY strange to me. After changing the code to h + (1.0 / 3.0), the breakpoint was no longer hit. 🤷‍♂

@yannickl
Copy link
Owner

I will update the code to reflect this, but it does not look like a robust fix (by default the decimal notation represent a Double instead of a Int, not a CGFloat). :/

@davidbjames
Copy link
Author

Oh yeah, I didn't think about that. I mean conceivably a person could overload operators to elide Double to CGFloat conversions and potentially the same bug would occur. It's super edge case, but possible.

What I really need to do is reproduce the bug and report it on https://bugs.swift.org/.

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

No branches or pull requests

2 participants