Improved output of formatNumberReadable

[Ruko97]

Follow up to issue #9601 and pull request #13163.
Based on advice by @cameel in the conversation in issue.
Previously formatNumberReadable(57896044618658097711785492504343953926634992332820282019728792003956564819968) would equal to 0x80 * 2 ** 248, but now it equals to 2 ** 255.
Also, there was a bug in the test case test_format_number_readable_signed in test/StringUtils.cpp. formatNumberReadable(-0xffffffff) should be equal to -2 ** 32 + 1 but previously it was tested to be equal to -2 ** 32 - 1. The new version of formatNumberReadable fixes this problem.

[matheusaaguiar]

Looks good!
Just some code style adjustments and readability suggestions.

[matheusaaguiar]

I think this is in good shape. Please, rebase.

[Ruko97]

Pushed a rebased version right now. Please check it.

[matheusaaguiar]

Sorry, I had another look and found some things that were glossed over before and could be improved.

[leonardoalt]

Auto please rebase onto origin/develop to get rid of the CI errors.

[Ruko97]

I have rebased and updated my commit based on the suggestions here. Sorry for being late.

[matheusaaguiar]

I have rebased and updated my commit based on the suggestions here. Sorry for being late.

No need to apologize, you are not late at all. You are actually quite timely with your responses. Thanks for contributing to the project :)

[matheusaaguiar]

I consider it ready. But it needs another rebase on top of develop since we are having a release today.

[matheusaaguiar]

It is going to need another rebase, but I am approving so someone else from the team can merge it later.

[Ruko97]

The errors in CircleCI seem to be related to not being able to find fmt::format. Does CircleCI not use the fmt library?

[cameel]

It's there but we added it as a dependency very recently so not all of our libs link to it yet. You have to add it to target_link_libraries in libsolutil/CMakeLists.txt. See how #13473 does it.

[matheusaaguiar]

Some adjustments and I wanted to confirm that you added the test case mentioned here.
Also, @nikola-matic are you satisfied with the current replacements of string concatenation or do you think that they are needed for the simple cases at lines 133, 203 and 218?

[nikola-matic]

Some adjustments and I wanted to confirm that you added the test case mentioned here. Also, @nikola-matic are you satisfied with the current replacements of string concatenation or do you think that they are needed for the simple cases at lines 133, 203 and 218?

The formatting looks good now, and no, no need for fmt::format for simple cases. Another rebase would be nice to get rid of the gp2 failure.

[Ruko97]

Some adjustments and I wanted to confirm that you added the test case mentioned here. Also, @nikola-matic are you satisfied with the current replacements of string concatenation or do you think that they are needed for the simple cases at lines 133, 203 and 218?

Yes, I added them at lines 185, 186, 285 and 286. I added some more tests now just to make sure.

[Ruko97]

I am also removing the std::to_string functions, and rebasing the code to the latest version.

[nikola-matic]

@matheusaaguiar @ekpyron will this need a changelog entry?

[ekpyron]

@matheusaaguiar @ekpyron will this need a changelog entry?

I'd say, it's borderline. But strictly speaking it's user-facing, so we can just default to "yes".

[matheusaaguiar]

@Ruko97 , please add a Changelog entry under the Compiler Features section, describing shortly the improvement on readability of large/hex numbers. Also, the fixed bug, under section Bugfixes.

[nikola-matic]

@leonardoalt is this SMT checker related?

[nikola-matic]

Rebased, resolved conflicts, and added periods to changelog entires. The build should now be fine, and hopefully closer to merging.

[NunoFilipeSantos]

LGTM

[matheusaaguiar]

LGTM