Refactor/Modernise basics.cpp code

[olive-tree-branch]

Description

This pull request is a refactoring/modernisation of the basics.cpp code to help with maintainability and ease of understanding for the future. None of the changes have broken the functionality or needed the header file documentation to be updated. All changes were made in the source file and the header file wasn't touched.

Added headers:

• <ranges>
• <format>
• <string_view>

Functions changed:

• split
• oct_to_bin
• bin_to_oct
• oct_to_hex
• hex_to_oct
• is_hex/octal/binary/integer/double/number

Functions added:

• is_valid_input

The functions bin_to_hex and hex_to_bin were not changed as the dec_to_hex function is not in the basics.h file.
It might be worth investigating if moving it from networking.h to basics.h in the future.

Type of change

• Refactor/Modernise

How Has This Been Tested?

All changes have been tested with sktest and skunit_test and there are no new errors that show up and all tests are passing. I have also tested the changes with PR #167 and there are also no new errors and all tests are passing.

Checklist

• Tested with
• Tested with

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have requested a review for the pull request

[jankiluitel]

Thanks for the refactor — the code is noticeably simpler and easier to follow. I noticed one behaviour concern with the new is_valid_input approach.

is_integer may now accept malformed values such as 1+2, +-1, or --5, and is_number may accept values such as 1.2.3, because the helper only checks whether each character belongs to the allowed character set. The previous strtol/strtod implementation also handled sign placement and numeric structure more strictly.

Could you please add tests for these malformed inputs and ensure the refactor preserves the existing validation behaviour? It would also be worth confirming std::format compatibility with SplashKit’s supported compiler environments.

Other than that, the overall simplification looks good.

[olive-tree-branch]

Thanks for the feedback!

So what I ended up doing was changing is_number and is_integer back to their original code. I tried multiple different things however the closest was std::from_chars but behavior was a bit different so I decided to leave it for this point in time. I might continue to play with it but I'm happy with leaving the old code in.

As for std::format, all of the major compilers have implemented it and is fully supported if it's a C++20 project. One thing that I could add is a check for backwards compatibility in case there isn't support

[rory-cd]

Thanks for the update, it's a good initiative and I do think it's easier to read. I have tested in WSL and all tests passed.

Assuming C++20 should be ok since I believe SKM builds the main library for v20. I think your suggestion of using a fallback for older versions is a good one though.

One issue with this update is that some of the numerical conversions now indirectly use stoi. This can be an issue for larger inputs, since integer overflow could be an issue. I have tested this with the old and new implementation using this assertion:
REQUIRE(bin_to_oct("11111111111111111111111111111111") == "37777777777");

It passed before, but fails after implementing this PR, since the large input gets converted to an integer during conversion. I think it's worth addressing this possibility.

[jankiluitel]

Thanks for addressing the previous feedback. I reviewed the latest changes, and the updated binary-to-octal implementation now avoids converting the entire binary value through an intermediate integer, addressing the overflow concern raised in the previous review.

The additional input-validation edge cases are also a good improvement.

It would still be useful to include the large bin_to_oct case as a regression test to protect this behaviour in the future, but I don't consider that blocking for the current changes.

Everything else looks good from my side. Approved! ✅