Skip to content

Conversation

toughengineer
Copy link
Contributor

See details in #319.

The new functions use the same code that the existing functionality uses, so I don't know which tests should be added other than the most basic ones checking the correctness of the interface.

More documentation will be added later if needed.

There are unrelated changes leftover from development which are still useful IMHO,
I can roll them back if you don't want them.

@lemire
Copy link
Member

lemire commented Sep 2, 2025

@toughengineer

There are unrelated changes leftover from development which are still useful IMHO,
I can roll them back if you don't want them.

Please keep the pull request to a minimum. Do not refactor the code needlessly.

This is a thoroughly tested code base. The assumption I have when reviewing code is that you are likely to make things worse, not better.

If you think you can make it better, please use separate pull requests, and include benchmarks.

@toughengineer toughengineer force-pushed the int_multiplication_by_power_of_10 branch from 3d9d012 to cc90f24 Compare September 2, 2025 20:06
@toughengineer

This comment was marked as resolved.

@lemire

This comment was marked as resolved.

@toughengineer
Copy link
Contributor Author

@lemire, so, are you interested in such functionality in this library?

If yes, I will add draft docs in the README and whatnot, and ensure CI passes and there is no degradation in performance.
What else should I add or care for?

@lemire
Copy link
Member

lemire commented Sep 3, 2025

@toughengineer Please see toughengineer#1

so, are you interested in such functionality in this library?

This is a community-driven project. If you complete it a bit with documentation, and if it seems solid, then I will merge it.

Note that I ran benchmarks and your small changes do not appear to affect (much) the performance of our main functions.

@toughengineer
Copy link
Contributor Author

If you complete it a bit with documentation, and if it seems solid, then I will merge it.

Got it.

@lemire, what do you think about the name multiply_integer_and_power_of_10()?
I think it can be improved but I really struggle to come up with something better, ideas are very welcome.

and it seems to bring performance to the level before the changes somewhat
@lemire
Copy link
Member

lemire commented Sep 3, 2025

@toughengineer

Possible names:

  • times_pow10
  • integer_times_pow10
  • multiply_by_pow10
  • pow10_scale

My favourite is times_pow10.

What do you think?

@toughengineer
Copy link
Contributor Author

Of these I like integer_times_pow10 the most because it explicitly mentions integer and is potentially less misleading.

Searching "times", "times_pow" and "timespow" in C++ code on GitHub shows that it is something that people use yielding results like "smthng_times_2", "smthng_time_pow_of_2", "SmthngTimesPowerOfTwo" and the like.

Thank you for ideas, gotta think about that.

@toughengineer toughengineer marked this pull request as ready for review September 5, 2025 10:37
@toughengineer
Copy link
Contributor Author

Added more changes, I think it's ready for review.

Things to pay attention to:

  • renamed the function to integer_times_pow10(), this also allows to potentially have an inverse function named like to_integer_times_pow10(), (counter)arguments are welcome;
  • review the docs for understandability and clarity, notes are welcome;
  • slightly reworked tests, are these enough?

Unfortunately we generally can't construct a double literal with preprocessor,
something like

MACRO(1, , 1) // produces 1e1
MACRO(1, -, 1) // produces 1e-1

is too ugly,
as much as I don't like to involve from_chars() which is itself under test, this is the only practical way.

@lemire
Copy link
Member

lemire commented Sep 5, 2025

@toughengineer

We are getting build errors:


/home/runner/work/fast_float/fast_float/tests/basictest.cpp: In instantiation of ‘void verify_integer_multiplication_by_power_of_10(Int, int, double) [with Int = long long unsigned int]’:
/home/runner/work/fast_float/fast_float/tests/basictest.cpp:2119:47:   required from ‘void verify_integer_multiplication_by_power_of_10(Int, int) [with Int = long long unsigned int]’
/home/runner/work/fast_float/fast_float/tests/basictest.cpp:2224:49:   required from here
/home/runner/work/fast_float/fast_float/tests/basictest.cpp:2094:38: error: call of overloaded ‘integer_times_pow10(long long unsigned int&, int&)’ is ambiguous
 2094 |       fast_float::integer_times_pow10(mantissa, decimal_exponent);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/runner/work/fast_float/fast_float/include/fast_float/fast_float.h:78,
                 from /home/runner/work/fast_float/fast_float/tests/basictest.cpp:5:
/home/runner/work/fast_float/fast_float/include/fast_float/parse_number.h:348:1: note: candidate: ‘double fast_float::integer_times_pow10(uint64_t, int)’
  348 | integer_times_pow10(uint64_t mantissa, int decimal_exponent) noexcept {
      | ^~~~~~~~~~~~~~~~~~~
/home/runner/work/fast_float/fast_float/include/fast_float/parse_number.h:360:1: note: candidate: ‘double fast_float::integer_times_pow10(int64_t, int)’
  360 | integer_times_pow10(int64_t mantissa, int decimal_exponent) noexcept {
      | ^~~~~~~~~~~~~~~~~~~
/home/runner/work/fast_float/fast_float/include/fast_float/parse_number.h:377:1: note: candidate: ‘double fast_float::integer_times_pow10(unsigned int, int)’
  377 | integer_times_pow10(unsigned mantissa, int decimal_exponent) noexcept {
      | ^~~~~~~~~~~~~~~~~~~
/home/runner/work/fast_float/fast_float/include/fast_float/parse_number.h:382:1: note: candidate: ‘double fast_float::integer_times_pow10(int, int)’
  382 | integer_times_pow10(int mantissa, int decimal_exponent) noexcept {
      | ^~~~~~~~~~~~~~~~~~~

@toughengineer
Copy link
Contributor Author

Apparently int64_t, uint64_t, long long and unsigned long long are four distinct types as far as GCC and Clang are concerned.
(Not with MSVC conventions though.)

@lemire
Copy link
Member

lemire commented Sep 6, 2025

It is fine if we don't implement going to float or float16_t and friends, but we want to know for sure that the API is extensible.

@toughengineer
Copy link
Contributor Author

I meant that given

double integer_times_pow10(std::integral auto, int) {/*implementation*/}

to extend it to be able to get different floating point types as return types you would reimplement it in the following way:

template<std::floating_point F>
F integer_times_pow10(std::integral auto, int) {/*implementation*/}

double integer_times_pow10(std::integral auto m, int e) { return integer_times_pow10<double>(m, e); }

(I use concepts as shorthand.)

This way you still can call the ordinary version and get double, as well as templated version:

double d = integer_times_pow10(12, 34);
float f = integer_times_pow10<float>(12, 34);
float16_t f16 = integer_times_pow10<float16_t>(12, 34);

Since you have this concern now, I will implement it in the next PR so the solution is complete and tidy.

Or is there something else that you have in mind, @lemire?

@lemire
Copy link
Member

lemire commented Sep 18, 2025

Or is there something else that you have in mind, @lemire?

Sorry for the delay.

Merging. This will be part of the next release.

@lemire lemire merged commit e20c952 into fastfloat:main Sep 18, 2025
36 checks passed
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