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

Make FixedDecimal's round to increment methods experimental #4270

Merged

Conversation

jedel1043
Copy link
Contributor

No description provided.

@@ -1082,13 +1091,94 @@ impl FixedDecimal {
/// dec.trunc(0);
/// assert_eq!("1", dec.to_string());
/// ```
#[cfg(not(feature = "experimental"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's not do this, write a private trunc_to_increment_inner function and then call them from both

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would doing something like this also work for you?

pub fn expand(&mut self, position: i16) {
    #[cfg(feature = "experimental")]
    {
        self.expand_to_increment(position, RoundingIncrement::MultiplesOf1)
    }

    #[cfg(not(feature = "experimental"))]
    {
        // Original impl
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I don't want us to duplicate the impl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll assume that's a yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that still duplicates the impl, no?

Copy link
Contributor Author

@jedel1043 jedel1043 Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the reason we're making the methods experimental is to benchmark them, so we need to preserve the original implementation somehow, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, are these impls distinct?

In that case: we should have internal methods for all of the implementations and use cfgs on thin wrappers

basically we should not do a big cfg/cfg-not pairing for this kind of thing, it's really hard to read. The actual meat of the impl should be in its own method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair! In that case I'll extract the implementation into an internal method and call that from the public method.

@jedel1043 jedel1043 force-pushed the experimental-rounding-increments branch from 5f83c85 to c162842 Compare November 10, 2023 23:02
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I don't like landing code with #[cfg(not(...))] because it is not tested in CI but it's okay in this case since it's between experimental and non-experimental, and we have a path to removing it soon.

@jedel1043 jedel1043 force-pushed the experimental-rounding-increments branch from c162842 to 357bc19 Compare November 11, 2023 00:16
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally we'd have RoundingIncrement be internally available (reexported as public in experimental mode) so that we don't need a separate internal function, but this is fine for now

@Manishearth Manishearth merged commit 4a6df7b into unicode-org:main Nov 11, 2023
@jedel1043 jedel1043 deleted the experimental-rounding-increments branch November 11, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants