typst / ecow

Compact, clone-on-write vector and string.
Apache License 2.0
208 stars 16 forks source link

Inline size for 32-bit systems #12

Closed CosmicHorrorDev closed 1 year ago

CosmicHorrorDev commented 1 year ago

I was doing some testing (got a lot planned for that btw), and I noticed that the test_mem_size test fails

#[test]
fn test_mem_size() {
    let word = mem::size_of::<usize>();
    assert_eq!(mem::size_of::<EcoVec<u8>>(), 2 * word);
    assert_eq!(mem::size_of::<Option<EcoVec<u8>>>(), 2 * word);

    if cfg!(target_endian = "little") {
        assert_eq!(mem::size_of::<EcoString>(), 2 * word);    // <-- Fails here. left: 16, right: 8
        assert_eq!(mem::size_of::<Option<EcoString>>(), 3 * word);
    }
}

It looks like the inline limit on 32-bit LE systems is still 15. I'd love to submit a PR to fix things, but I wasn't sure if the right approach was to use a smaller limit on 32-bit systems or to keep a larger limit even though the word size is smaller

laurmaedje commented 1 year ago

Yeah, forgot about that test. I think keeping the limit at 15 for 32-bit makes more sense, 7 just isn't enough.

CosmicHorrorDev commented 1 year ago

I should probably add it to CI too, but it's pretty easy to test different arches with Miri

$ cargo +nightly miri test --target i686-unknown-linux-gnu     # 32-bit
$ cargo +nightly miri test --target sparc64-unknown-linux-gnu  # 64-bit BE

It didn't pick up on any UB with either of those, so you've been doing good!

I'll open a PR to fix that test later today

laurmaedje commented 1 year ago

Good to know!