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

Add impl DataTypeKind for size types (usize/isize) #689

Closed
Ararem opened this issue Dec 25, 2022 · 3 comments · Fixed by #693
Closed

Add impl DataTypeKind for size types (usize/isize) #689

Ararem opened this issue Dec 25, 2022 · 3 comments · Fixed by #693

Comments

@Ararem
Copy link
Contributor

Ararem commented Dec 25, 2022

Description

When using a slider, you need to pass in a value that implements DataTypeKind. Currently, this is only the primitive numeric types, and I propose to extend it to include usize and isize as well.

Proposal

We add an

unsafe impl DataTypeKind for usize {
    const KIND: DataType = <PLATFORM DEPENDENT>;
}

We may need to add some internal casting to whichever integer type it matches. I assume this could be done by conditional compilation, or size_of: const USIZE_WIDTH:usize = size_of::<usize>();

Alternatives

My current implementation is to cast to u64 everywhere:

        // usize can't be used in a slider, so we have to cast to u64, use that, then case back
        type SliderType = u64; // Might fail on 128-bit systems where usize > u64, but eh
        let mut num_track_frames_compat: SliderType = *track_frames as SliderType;
        ui.slider_config(
            "Num Tracked Frames",
            1,
            Self::HARD_LIMIT_MAX_FRAMES_TO_TRACK as SliderType,
        )
        .flags(SliderFlags::LOGARITHMIC)
        .build(&mut num_track_frames_compat);
        *track_frames = num_track_frames_compat as usize;
        /*...*/

        *displayed_frames = min(*displayed_frames, *track_frames); // Don't allow it to go over num_track_frames
        let mut num_displayed_frames_compat: SliderType = *displayed_frames as SliderType; // Might fail on 128-bit systems, but eh
        ui.slider_config(
            "Num Displayed Frames",
            1,
            min(Self::HARD_LIMIT_MAX_FRAMES_TO_TRACK, *track_frames) as SliderType,
        )
        .flags(SliderFlags::LOGARITHMIC)
        .build(&mut num_displayed_frames_compat);
        *displayed_frames = num_displayed_frames_compat as usize;
        /*...*/

Safety & Reliability

As far as I'm aware, usize changes depending on which platform it's being targeted at. On 32-bit systems, it's equivalent to u32, on 64-bit u64, etc. (Untested) solution:
Modify src/internals.rs to include these lines:

unsafe impl DataTypeKind for usize {
    #[cfg(target_pointer_width = "8")]
    const KIND: DataType = DataType::U8;

    #[cfg(target_pointer_width = "16")]
    const KIND: DataType = DataType::U16;

    #[cfg(target_pointer_width = "32")]
    const KIND: DataType = DataType::U32;

    #[cfg(target_pointer_width = "64")]
    const KIND: DataType = DataType::U64;

    // Fallback for when we are on a weird system width
    //
    #[cfg(not(any(
    target_pointer_width = "8",
    target_pointer_width = "16",
    target_pointer_width = "32",
    target_pointer_width = "64"
    )))]
    compile_error!("cannot impl DataTypeKind for usize: unsupported target pointer width. supported values are 8, 16, 32, 64");
}

unsafe impl DataTypeKind for isize {
    #[cfg(target_pointer_width = "8")]
    const KIND: DataType = DataType::I8;

    #[cfg(target_pointer_width = "16")]
    const KIND: DataType = DataType::I16;

    #[cfg(target_pointer_width = "32")]
    const KIND: DataType = DataType::I32;

    #[cfg(target_pointer_width = "64")]
    const KIND: DataType = DataType::I64;

    // Fallback for when we are on a weird system width
    //
    #[cfg(not(any(
        target_pointer_width = "8",
        target_pointer_width = "16",
        target_pointer_width = "32",
        target_pointer_width = "64"
    )))]
    compile_error!("cannot impl DataTypeKind for isize: unsupported target pointer width. supported values are 8, 16, 32, 64");
}
@dbr
Copy link
Contributor

dbr commented Jan 4, 2023

I agree this would be good to support - I've occasionally run into the same tediousness of having to cast usize back-and-forth to u64

Can't think of any problems with the proposed solution - @thomcc am I missing anything obvious?

Would you have time to make a PR for this?

Could possibly remove the target_pointer_width = "8" case as I suspect a target with 8-bit pointer would likely run into other problems first, and I think parts of Rust itself assume pointers are either 16/32/64, like this random example

@dbr dbr added the enhancement label Jan 4, 2023
@Ararem
Copy link
Contributor Author

Ararem commented Jan 4, 2023

Yeah I agree, we probably don't need the 8width case, so we can probably remove that. I can make a PR if you want, just I'm not that well versed (read: absolutely terrible) withgit` so it might be a little funky. I'll try do it now and comment an update.

@Ararem
Copy link
Contributor Author

Ararem commented Jan 4, 2023

PR Created: #693, feel free to merge whenever

@dbr dbr linked a pull request Jan 5, 2023 that will close this issue
@dbr dbr closed this as completed in #693 Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants