Out of range bugfix

[olive-tree-branch]

Description

Fixed the std::out_of_range exception that was happening in the unit tests for the functions:

• bin_to_dec
• oct_to_dec

This problem that was causing the exception to be thrown was the call to std::stoi in the functions above which were returning an unsigned int. This was resolved by changing std::stoi to static_cast<unsigned int>(std::stoul(...)) to have the correct return type and avoid a -Werror-conversion flag.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Both sktest and skunit_tests were ran and no exceptions were thrown and no tests were failed for the two functions that were changed.

Testing Checklist

• Tested with sktest
• Tested with skunit_tests

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 (N/A)
• My changes generate no new warnings
• I have requested a review from ... on the Pull Request

[himanshigaba22]

Good catch and a clean fix - switching to stoul before narrowing to unsigned int correctly avoids the out_of_range thrown by stoi on values between INT_MAX and UINT_MAX. Two small things:

Since stoul only throws for values exceeding unsigned long, an input that overflows unsigned int but fits in unsigned long will now silently truncate instead of throwing -probably fine, but worth a quick comment noting that's accepted, or a range check if malformed input is a concern here.
Would it make sense to add a regression test covering the specific inputs that used to throw, so this doesn't regress silently in the future?

Also using std::stoul fully-qualified where the surrounding code calls stoi unqualified - minor, just flagging for consistency.

[olive-tree-branch]

Appreciate the feedback.

So I've added some checks to the functions that make sure that if inputs are going over the UINT boundary it will just return the UINT max value. I've added try blocks around the return calls in the case of an unexpected error and will just return 0.

As for the regression test I can check about that but I added some tests in to make sure the output values were correct for the checks that I added.

I also made the call to stoi underneath one of the functions fully qualified as all other calls to similar functions in the file that were fully qualified

[rory-cd]

General Information

Type of Change:

• Bug fix

Nice work! Truncating large values seems sensible. I tested on WSL and it seems to resolve the issue. Just a couple of minor things:

Requested changes:

• As @himanshigaba22 noted, the truncation is silent, so the function description/comment in basics.h should probably be updated so users know what to expect.
• The code formatting needs to be fixed up a little, e.g. if( should be if ( and some of the indenting is inconsistent (like the return statements).

Optional:
One other consideration is the digit-counting approach. Using if(bin_str.length() > 32) works, but may be a bit brittle (for example with leading zeroes). I'd suggest another method of clamping, like perhaps parsing to a long long and then clamping if it exceeds numeric_limits<unsigned int>::max(). If it can't be parsed into long long then it could be caught and handled (out of range exception).

Code Quality

• Repository: Thoth-Tech NOT SplashKit
• Readability: Is the code easy to read and follow? If not are there comments to help understand the code?
• Maintainability: Can this code be easily maintained or extended in the future?

Functionality

• Correctness: Does the code meet the requirements of the task?
• Impact on Existing Functionality: Has the impact on existing functionality been considered and tested?

Testing

• Test Coverage: Are unit tests provided for new or modified code?
• Test Results: Have all tests passed?

Documentation

• Documentation: Are both inline and applicable external documentation updated and clear?

Pull Request Details

• PR Description: Is the problem being solved clearly described?
• Checklist Completion: Have all relevant checklist items been reviewed and completed?

[olive-tree-branch]

Apologies for not adjusting the comment in the header file. I have adjusted them with the changes requested.

I'm not sure why there is a problem with the return statements being incorrectly formatted, I will look into it more as it might have to do with my tab settings

[jankiluitel]

Thanks for working on this bug fix. Replacing std::stoi with std::stoul and adding handling for values larger than unsigned int is a good improvement, and the additional boundary tests help verify the intended behaviour.

I did notice one inconsistency in the implementation:

bin_to_dec() and oct_to_dec() now catch exceptions and clamp values that exceed unsigned int::max(), but hex_to_dec() still directly calls std::stoi() without similar overflow handling. This means hexadecimal values outside the supported range could still throw an exception or behave differently from the binary and octal implementations.

For consistency across the conversion functions, I'd recommend updating hex_to_dec() to use the same pattern (std::stoul() with range checking and exception handling) unless there's a specific reason for the different behaviour.

Other than that, the changes look well structured and the new tests are a welcome addition.

[olive-tree-branch]

Thanks for the feedback.

I updated the hex_to_dec function to behave like the other functions do now and updated the documentation in the header file.

I also added tests for the hex_to_dec function as it turned out there wasn't any

[jankiluitel]

Thanks for addressing the review feedback. The conversion functions are now consistent in their overflow handling, and the additional tests provide good coverage for boundary and invalid input cases. The implementation looks good to me. Approved.