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

Replaced the numeric string conversion functions, and made the api for them consistent #4656

Closed
wants to merge 14 commits into from

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Jan 28, 2013

Right now, the string conversion functions for floats and integers are inconsistent with each other and the rest of the library, eg integers had str(n) for converting to base 10 and to_str(n, radix) for converting to a special base, while float had to_str(n, digits) which emitted a number in base 10 with up to digits trailing fractional digits etc. Additionally, float.rs had it's own conversion functions that parsed/emitted a ~str, uint-template.rs had an own implementation that used an stack allocated [u8], int-template simply delegated to the uint function, and f32 and f64 had no functions at all, instead relying on the user casting to/from float. This is my attempt at untangling this:

  • Moved all numeric modules in core into a sub folder. This has nothing to do with the code itself, but made working with the files a lot easier. (feel free to revert this)
  • (Central change) Added generic to_str_bytes() and from_str_bytes() functions to num.rs, along with some enums.
    • Both are highly configurable and should be able to parse/emit all integer types, floating point types, and anything else that implements the necessary traits, as well as reject them if they're invalid.
    • from_str_bytes takes a &[u8] and to_str_bytes returns a ~[u8]
    • from_str() and to_str() wrappers are provided to make working with &str and ~str easier.
    • For example, with this you could parse a positive-only base7 fractional number with binary exponent, and then emit it as a hexadecimal number with explicit positive sign and exactly 3 fractional digits.
  • For them to work, also added a few generic math functions and a Round trait + implementations, and new functions to char.rs.
  • Converted all numeric string conversion functions to use an unified naming convention and to wrap the new generic functions:
    • to_str() just takes the number itself, and returns an as-exact-as-possible base 10 representation
    • from_str() just takes a string, and returns Some(n) if it parses as a number in base 10, accepting also valid variations like an decimal exponent on floating point values, inf, Nan, or an explicit leading '+'.
    • to_str_radix() emits the number in a given base as exact as possible
    • from_str_radix() parses as a number in a given base but doesn't accept any special formats that could conflict (for example "inf").
  • Additionally, for floating point types the following functions are defined:
    • to_str_digits() returns a string with up to n fractional digits
    • to_str_exact() returns an string with exactly n fractional digits
    • to_str_hex() returns an hexadecimal floating point representation
    • from_str_hex() accepts an hexadecimal floating point representation with an optional binary exponent
  • All types also now implement the existing ToStr, FromStr traits at type-site, as well as the new ToStrRadix and FromStrRadix traits
    • They generally map to the stand-alone functions, but the ToStr implementation for floating point numbers only emits maximum 8 digits (using to_str_digits()) to make it more useful.

For backward compatibility, int-template.rs and uint-template.rs still have definitions for str() and some of the old functions for the local implementations that all now direct to the new generic ones. These should probably get thrown out in the long term, especially since half the other definitions changed anyway.

Also, all places changed by the floating point conversion should probably checked on whether they should uses limited digits, or the maximum calculable digits, especially the part of repr.rs that I had to change looks strange.

Additionally, while debugging I noticed that when librustc gets compiled somewhere to_str() gets called on an int or uint that gets counted up the powers of twos until it overflows (at least on 32bit), not sure if thats intentional. The behavior of the old functions in such a case was just to overflow silently and emit an wrong string, so...

As far as I know, make check passes completely, but after the rebase the test suite errors on test private::at_exit::test_at_exit ... with

coretest.stage2-i686-unknown-linux-gnu: /home/marvin/dev/rust/fork/rust/src/rt/rust_kernel.cpp:426: void rust_kernel::register_exit_function(spawn_fn, fn_env_pair*): Assertion `runner == at_exit_runner && "there can be only one at_exit_runner"' failed.

and doesn't complete because of that, so I can't be totally sure.

This should close the Issues #2608, #2649 (those functions no longer exits), #4126, #1784, #4314 and #4288.

I haven't looked at bigints yet, so I don't know if it would be useful there.

Lastly, as the new string conversion functions grew over time till they worked in all corner cases, performance might be considerably worse compared to the original versions, but I'm not sure if that's even a problem or how to tell.

@brson
Copy link
Contributor

brson commented Jan 29, 2013

This looks fantastic. Thanks for trying to sort it out. I'm going to run the tests here and see if I can reproduce that assert failure.

@Kimundi
Copy link
Member Author

Kimundi commented Jan 29, 2013

Just noticed an issue with to_str_radix with floats, it currently can produce "inf" both as the numeric or the special value. Have to find a solution that doesn't require the user to unpack an tupel to differentiate the two...

@brson
Copy link
Contributor

brson commented Jan 30, 2013

When I ran make check I hit minor syntax errors in stdtest and run-pass/core-export-f64-sqrt.rs. run-pass/syntax-extension-fmt.rs also has an assertion failure.

Can you run it through make check again to clean up these stragglers?

@Kimundi
Copy link
Member Author

Kimundi commented Jan 30, 2013

Those are probably cases where the testsuite crashed on me before reaching them. Have to do an rebase to see if make check completes for me now.

@Kimundi
Copy link
Member Author

Kimundi commented Jan 30, 2013

Okay, a rebase still gave me the same error while running coretest... Trying with a fresh clone right now.

@Kimundi
Copy link
Member Author

Kimundi commented Jan 30, 2013

Even a fresh clone and build of incoming without any of my changes produces

...
test path::tests::test_windows_paths ... ok
test pipes::test::test_oneshot ... ok
test pipes::test::test_peek_terminated ... ok
test pipes::test::test_select2 ... ok
test private::at_exit::test_at_exit ... coretest.stage2-i686-unknown-linux-gnu: /home/marvin/dev/rust/git/rust/src/rt/rust_kernel.cpp:426: void rust_kernel::register_exit_function(spawn_fn, fn_env_pair*): Assertion `runner == at_exit_runner && "there can be only one at_exit_runner"' failed.
make: *** [check-stage2-T-i686-unknown-linux-gnu-H-i686-unknown-linux-gnu-core-dummy] Abgebrochen

while running coretest. Either my system broke in general, or something recent broke 32bit linux

@brson
Copy link
Contributor

brson commented Jan 30, 2013

That is a new test case. I'll try a 32-bit build and see if I can reproduce.

@brson
Copy link
Contributor

brson commented Jan 31, 2013

I wasn't able to reproduce on i686-unknown-linux-gnu with optimizations on.

@Kimundi have you run make clean recently or built in a fresh workspace? The dependencies for the runtime files are wrong and can cause strange crashes sometimes when they don't rebuild.

There's no reason to delay because of this mystery, I think. I'll make sure everything passes here and push to the try bots.

@Kimundi
Copy link
Member Author

Kimundi commented Jan 31, 2013

@brson Thanks for fixing it on my behalf. It was a completely fresh directory, git clone <mozilla rust>, ./configure and make check. It build llvm and everything, it really is a mystery to me. :(

@brson
Copy link
Contributor

brson commented Jan 31, 2013

I opened #4711 about this test failure. I have an idea of what the problem might be. If my hunch is right then the pointers in that failing assert will be pointers to two different functions, both called 'exit_runner'.

@brson
Copy link
Contributor

brson commented Feb 1, 2013

@Kimundi I started working through this and found that the errors in run-pass/syntax-extension-fmt.rs were deeper than I wanted to get into. Specifically assert fmt!("%.0f", 5.892) == ~"6"); fails, producing "6." instead of "6". Can you look at that test specifically and resolve the errors there? You can run it with make check-stage2-rpass TESTNAME=syntax-extension-fmt

@brson
Copy link
Contributor

brson commented Feb 1, 2013

For your reference, the function used by fmt! for converting floats: https://github.com/mozilla/rust/blob/incoming/src/libcore/extfmt.rs#L563

@Kimundi
Copy link
Member Author

Kimundi commented Feb 1, 2013

Gonna look into it.

@Kimundi
Copy link
Member Author

Kimundi commented Feb 2, 2013

@brson Fixed all remaining errors, I'm going to rebase it one more time and then it's hopefully done.

Reason: Better grouping of related modules, future-proving for a more extensive math library.
Also fixes previous commit not compiling due to not finding Option.
They unify the different implementations that exists in int-template.rs, uint-template.rs and float.rs into one pair of functions, which are also in principle usable for anything that implements the necessary numeric traits. Their usage is somewhat complex due to the large amount of arguments each one takes, but as they're not meant to be used directly that shouldn't be a problem.
- Moved ToStr implementation of integers to int-template.rs.
- Marked the `str()` function as deprecated.
- Forwarded all conversion functions to `core::num::to_str_common()`
  and `core::num::from_str_common()`.
- Fixed most places in the codebase where `to_str()` is being used.
- Added int-template to_str and from_str overflow tests.
- Moved ToStr implementation of unsigned integers to uint-template.rs.
- Marked the `str()` function as deprecated.
- Forwarded all conversion functions to `core::num::to_str_common()`
  and `core::num::from_str_common()`.
- Fixed most places in the codebase where `to_str()` is being used.
- Added uint-template to_str and from_str overflow tests.
…ions.

Also fixed all conflicting calls of the old functions in the rest of the codebase.

The set of string conversion functions for each float type now consists of those items:
- to_str(), converts to number in base 10
- to_str_hex(), converts to number in base 16
- to_str_radix(), converts to number in given radix
- to_str_exact(), converts to number in base 10 with a exact number of trailing digits
- to_str_digits(), converts to number in base 10 with a maximum number of trailing digits
- implementations for to_str::ToStr and num::ToStrRadix
- from_str(), parses a string as number in base 10 including decimal exponent and special values
- from_str_hex(), parses a string as a number in base 16 including binary exponent and special values
- from_str_radix(), parses a string as a number in a given base excluding any exponent and special values
- implementations for from_str::FromStr and num::FromStrRadix
@Kimundi
Copy link
Member Author

Kimundi commented Feb 3, 2013

Rebased it and made it pass make check. This should be good to merge now.

Calling it on a special value now causes a failure, however `to_str_radix_special()` is provided which can be
used if those values are expected, and which returns a tupel to allow differentating them.
@brson
Copy link
Contributor

brson commented Feb 4, 2013

Merged. Thanks and great work, Kimundi.

@brson brson closed this Feb 4, 2013
bors pushed a commit to rust-lang-ci/rust that referenced this pull request May 15, 2021
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.

2 participants