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

We need a named constant for 9223372036854775808 #79

Open
justanotherfoundry opened this issue Nov 15, 2022 · 8 comments
Open

We need a named constant for 9223372036854775808 #79

justanotherfoundry opened this issue Nov 15, 2022 · 8 comments

Comments

@justanotherfoundry
Copy link
Contributor

justanotherfoundry commented Nov 15, 2022

It seems Glyph internally uses 9223372036854775808, or LLONG_MAX as ObjC calls it, in certain scenarios to indicate “no value” or similar. I believe it would be much cleaner to have a named constant for this, and proper documentation.

For example, in my ObjC code, I am checking the return value of kerningForFontMasterID:LeftKey:RightKey:direction: against LLONG_MAX to determine whether a kerning pair exists. This has always felt like a dirty hack as it is undocumented and doesn’t use a clearly named constant. The Python wrapper simply uses if value > 1000000:. That’s even worse IMHO, as if the value came from an unreliable external source, and needs a sanity check.

The unit tests check some values against 9223372036854775807 (that’s a 7 at the end, i.e. LLONG_MAX-1). Why is this? What is the meaning?

Can I kindly ask to solve this in a cleaner, less hacky way by simply providing a named constant (in ObjC as well as Python). Thanks! – Tim

@florianpircher
Copy link

9223372036854775807 is NSNotFound, a constant that Apple’s APIs are frequently using to indicate that there is no normal value (since in Objective-C ints, points, etc. cannot be nil).

@justanotherfoundry
Copy link
Contributor Author

Thanks, that’s interesting.

This means:

  • In some cases, Glyphs internally uses LLONG_MAX.
  • In some cases, Glyphs internally uses NSNotFound (which is different from LLONG_MAX). At least, this is what the Python unit tests suggest.
  • Both of this is undocumented.
  • The Python object wrapper checks against 1000000.

That’s not satisfying, is it?

@florianpircher
Copy link

In some cases, Glyphs internally uses NSNotFound (which is different from LLONG_MAX)

Where are you getting 9223372036854775808? LLONG_MAX is also 9223372036854775807.

@justanotherfoundry
Copy link
Contributor Author

justanotherfoundry commented Nov 16, 2022

Now, that’s confusing. It took me a while to solve that puzzle:

kerningForFontMasterID:LeftKey:RightKey:direction: returns a CGFloat a.k.a. double
• in my code, checking that returned double against LLONG_MAX a.k.a. NSNotFound returns true
• printing that value shows 9223372036854775808, i.e. NSNotFound + 1

Can’t be? Yes, it can. The reason is – as far as I figured out – that NSNotFound can’t be stored in a double. Apparently the method tries to return NSNotFound but it actually returns NSNotFound + 1. During my test for equality, NSNotFound is lossily cast to a double, this is why the values appear to be identical even though they aren’t. So, I was unconsciously testing whether the method returns a lossy cast of NSNotFound by comparing it to another lossy cast of NSNotFound.

To me, this shows how hacky and fragile the current solution is. I feel uncomfortable working like this.

@florianpircher
Copy link

florianpircher commented Nov 16, 2022

The workaround is to check whether a value is equal to or less than NSNotFound. Still hacky, but there is no good solution to this in Objective-C. NaN values have their own set of problems, especially when working with Apple APIs. CGFLOAT_MAX works in some cases, but is unusual as a ‘nothing’ value on Apples platforms.

@schriftgestalt
Copy link
Owner

Comparing floats is always a bit tricky. That's why I picked a (random) big (but smaller than NSNotFound) number as a threshold in the python code. As far as I understand, kerning values are stored as Int16, so the max value is 0x7FFF. So any value between that and NSNotFound is fine as threshold.

@justanotherfoundry
Copy link
Contributor Author

justanotherfoundry commented Nov 21, 2022

Thanks for the explanations. I wouldn’t say comparing floats is always tricky, though. It would have been easy to define a large integer value (one that can be represented by a 32-bit float) as a named constant, let’s say NO_KERNING_VALUE or UNSPECIFIED_VALUE, then all code could safely check for equality instead of using a random threshold. This would make the code robust and readable.

@schriftgestalt
Copy link
Owner

It might be better wrap the relevant methods so that they return None instead of NSNotFound.

But we can add a constant like MAX_KERN that you need to compare with a < instead of ==.

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

3 participants