[read-fonts] fix ClassDefFormat2::get()

[dfrg]

The conditional logic for selecting the appropriate class record was not quite right leading to incorrect class values being returned. This implements the lookup using a binary search and adds a test for correct behavior.

JMM

[cmyr]

oops, good catch! Maybe an argument for writing things like this as, (record.start_gid()..=record.end_gid()).contains(gid), which is easier to grok.

Also a number of years back I wrote up some simple benchmarks comparing binary and linear search for an integer over a range of collection sizes, and although I don't recall the exact numbers, linear search was winning up to some reasonable collection length, like ~16, and my conclusion was that if you know your collection length is unlikely to be huge than linear is preferable. (Now I'm curious, and maybe I should go recreate that code and find out if I'm remebering correctly)

[dfrg]

oops, good catch! Maybe an argument for writing things like this as, (record.start_gid()..=record.end_gid()).contains(gid), which is easier to grok.

Good call! Changed to use Range::contains for the final check.

Also a number of years back I wrote up some simple benchmarks comparing binary and linear search for an integer over a range of collection sizes, and although I don't recall the exact numbers, linear search was winning up to some reasonable collection length, like ~16, and my conclusion was that if you know your collection length is unlikely to be huge than linear is preferable. (Now I'm curious, and maybe I should go recreate that code and find out if I'm remebering correctly)

A lot of ranged based classdef (and coverage) tables have a handful of entries and I'm definitely willing to believe a linear search is faster for some n (and 16 sounds like the right ballpark). This is probably an area worth doing some perf exploration in the future as these tables are going to be in heavy use in a shaper.