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 trait UnsignedAbs #315

Open
emilk opened this issue Apr 2, 2024 · 3 comments
Open

Add trait UnsignedAbs #315

emilk opened this issue Apr 2, 2024 · 3 comments

Comments

@emilk
Copy link

emilk commented Apr 2, 2024

All signed rust integer types has an unsgined_abs function, e.g. (-7_i32).unsigned_abs() == 7_u32, which is there to correctly handle i32::MIN (which cannot be negated as a i32).

I suggest we either add this to PrimInt (returning identify for unsigned integers), or add a new UnsignedAbs trait for this.

I'd be happy to make a PR if this sounds good.

@cuviper
Copy link
Member

cuviper commented Apr 2, 2024

I think it will have to be a separate trait, especially because it needs an associated type for the unsigned return.

I fear that associated type will also make this a bear to actually use in generic code though. Have you experimented with a local trait doing this?

@emilk
Copy link
Author

emilk commented Apr 3, 2024

You are right that a separate trait makes most sense.

I tried this in a real world example, and it works great:

pub trait UnsignedAbs {
    /// An unsigned type which is large enough to hold the absolute value of `Self`.
    type Unsigned;

    /// Computes the absolute value of `self` without any wrapping or panicking.
    fn unsigned_abs(self) -> Self::Unsigned;
}

impl UnsignedAbs for i64 {
    type Unsigned = u64;
    fn unsigned_abs(self) -> Self::Unsigned {
        self.unsigned_abs()
    }
}

impl UnsignedAbs for isize {
    type Unsigned = usize;
    fn unsigned_abs(self) -> Self::Unsigned {
        self.unsigned_abs()
    }
}

/// Pretty format a signed number by using thousands separators for readability.
pub fn format_int<Int>(number: Int) -> String
where
    Int: Display + PartialOrd + num_traits::Zero + UnsignedAbs,
    Int::Unsigned: Display + num_traits::Unsigned,
{
    if number < Int::zero() {
        format!("-{}", format_uint(number.unsigned_abs()))
    } else {
        add_thousands_separators(&number.to_string())
    }
}

/// Pretty format an unsigned integer by using thousands separators for readability
#[allow(clippy::needless_pass_by_value)]
pub fn format_uint<Uint>(number: Uint) -> String
where
    Uint: Display + num_traits::Unsigned,
{
    add_thousands_separators(&number.to_string())
}

/// Add thousands separators to a number, every three steps,
/// counting from the last character.
fn add_thousands_separators(number: &str) -> String {}

@emilk emilk changed the title Add unsigned_abs to PrimInt Add trait UnsignedAbs Apr 3, 2024
emilk added a commit to rerun-io/rerun that referenced this issue Apr 4, 2024
### What
Mostly as an experiment to see if this makes sense as a `trait` for
`num-traits` (see rust-num/num-traits#315)

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[rerun.io/viewer](https://rerun.io/viewer/pr/5754)
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/5754?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/5754?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5754)
- [Docs
preview](https://rerun.io/preview/b55246f04976986ca8ffac7b72963eae454e374b/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/b55246f04976986ca8ffac7b72963eae454e374b/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@cuviper
Copy link
Member

cuviper commented May 3, 2024

I think it should probably take &self for consistency, especially compared to Signed::abs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants