Instead of allocating a new HashSet, we could probably just pass in a reference to the HashMap directly, and use HashMap::contains_key? One possible downside of this is that it might make calc_code_page_ranges have a slightly weird signature, and harder to use from other places?

I'm going to move this function so it becomes a method of the os2 table since it makes more sense for the code to live there. I may just end up making the method signature take the cmap table as input but I'll think about it.

In addition to the .cloned() method on Iterator, there's also a copied() method. In practice these will behave identically, but using copied() where possible makes it clearer to the reader that this is a cheap operation.

A few other ways to write this:

let unicodes: HashSet<_> = mapping.keys().copied().collect();
let unicodes = mapping.keys().copied().collect::<HashSet<_>>();

Thanks!

Which approach would you say is the best to adopt for the future? personally I find the turbofish style a little weird but for all I know this may be the most common.

I personally probably do the second form more often, but I wouldn't comment on the first form in code review 🤷

a few things:

as written, this is doing some extra work.

• unwrap_or: this always generates the code passed in, so we will run calc_code_page_ranges even if we already have the ranges. The alternative is unwrap_or_else, which takes a closure that is evaluated lazily.
• iter..collect: calc_code_page_ranges returns exactly the Vec<u8> that we want, but here we're then iterating it again to create a copy.
• if we're only using unicodes in the case where the code page ranges don't exist, we should avoid creating it unless necessary.

Put all together, I would write this as something like:

let code_pages = info.open_type_os2_code_page_ranges
        .clone() // clone on an option clones the contents if it exists, otherwise does nothing
        .unwrap_or_else(|| { // only called in the `None` case
            // type should be inferred based on the function we pass it to?
            let unicodes = mapping.keys().copied().collect();
            calc_code_page_ranges(&unicodes)
        });

Awesome thanks!

I would write this as,

let mut code_pages = code_pages; // unnecessary, just give this a mut binding above
code_pages.sort_unstable(); // not necessary if already sorted
let split_at = code_pages.iter().position(|x| x >= 32).unwrap_or_else(|| code_pages.len());
let mut code_pages2 = code_pages.split_off(split_at);
code_pages2.iter_mut().for_each(|x| x -= 32);

• Is there a reason to prefer HashSet to Vec, here?
• would it make sense for us to generate the two separate vecs here, instead of doing it at the call-site?

Is there a reason to prefer HashSet to Vec, here?

You're right, this could and probably should be a vec.

would it make sense for us to generate the two separate vecs here, instead of doing it at the call-site?

We could even go a step further and return a tuple containing two 32bit ints which is the desired end result.

oh totally, that's the sort of thing that rust excels at, and then we're doing way less work.

I would be tempted to write this as,

let has_ascii = (0x20..0x7E).all(|x| unicodes.contains(&x));

This saves us one more allocation. 🤷

SGTM

now I'm just having fun, but I would be tempted to at least handle this simple case of "check char and addd to table" with a macro:

some time later:

Okay I'm going to not let myself rabbit-hole on this too hard, but as an illustration: with the following macro, I can replace this section with:

macro_rules! add_page_if_needed {
    // helper for if there's no condition and no block
    ($collection:ident, $ranges:ident, $chr:expr, $page:expr) => {
        add_page_if_needed!($collection, $chr, $page, true, {});
    };
    // helper for if there's a condition but no block
    ($collection:ident, $ranges:ident, $chr:expr, $page:expr, $constraint:expr) => {
        add_page_if_needed!($collection, $ranges, $chr, $page, $constraint, {});
    };
    // the verbose form
    ($collection:ident, $ranges:ident, $chr:expr, $page:expr, $constraint:expr, $block:block) => {
        if $collection.contains(&($chr as u32)) && $constraint {
            $ranges.insert($page);
            $block;
        }
    };
}

In any case: this is provided more for illustrative purposes, and rust macros can be confusing, but I wanted to point this out as a way I would think about this problem. :)

So now that I have a clearer idea of what we're doing, it feels like what we really want here is a function that creates two 32-bit integers in place. This is the sort of low-level thing that can often be annoying to do, but with Rust we could make this quite easy. What I would personally do is make a helper type that looks like a collection but is just doing bit arithmetic under the hood, something like:

#[derive(Default)]
struct CodePageRanges {
    one: u32,
    two: u32,
}

impl CodePageRanges {
    fn set(&mut self, idx: usize) {
        assert!(idx < 64);
        if idx >= 32 {
            self.two |= 1 << (idx - 32);
        } else {
            self.one |= 1 << idx;
        }
    }
    
    fn into_inner(self) -> (u32, u32) {
        (self.one, self.two)
    }
}

And then you can just call this directly, everwhere that you are using code_page_ranges:

let mut code_page_ranges = CodePageRanges::default();

if unicodes_contains('Þ') && has_ascii {
    code_page_ranges.set(0); // Latin 1
}

This then does everything at once; we're building up our final representation directly, as we add each new value.

Nice! this is much cleaner.