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

stdlib: improve string to integer conversion functions #381

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

jmaksymowicz
Copy link
Contributor

@jmaksymowicz jmaksymowicz commented Sep 9, 2024

Add errno setting in strto{u}ll, fix minor bugs in strto{u}l. Details in commit messages.

Motivation and Context

Closes phoenix-rtos/phoenix-rtos-project#543

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

@jmaksymowicz jmaksymowicz self-assigned this Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

Unit Test Results

7 949 tests  ±0   7 231 ✅ ±0   40m 19s ⏱️ - 1m 36s
  461 suites ±0     718 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit e2d34ae. ± Comparison against base commit 2133985.

♻️ This comment has been updated with latest results.

stdlib/strtoull.c Outdated Show resolved Hide resolved
c -= 'a' - 10;
else
}
else {
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errno should be set to EINVAL if no conversion could be performed. This is an optional error condition, so I'm not 100% sure it will be useful in portable applications, but maybe it is worth including.

I have noticed that strtoul.c and strtoull.c still differ and are not unified/commonized completely and that's why EINVAL is not set - is this intended (for now) to avoid a large refactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with glibc and it doesn't set errno to EINVAL if no conversion can be performed - so perhaps it won't be very useful, but I added it.

Making code for strtol and strtoll common would be tricky, because (at least on 32-bit platforms) they use different arithmetic types for intermediate results. If we used unsigned long long for both of them, it would reduce performance if 32-bit conversion is requested. Although perhaps the impact wouldn't be large, because only addition and multiplication is used.

Add setting of errno on underflow, overflow and wrong base.
Unify common code between strtoll and strtoull.
Make code MISRA compliant.

JIRA: RTOS-869
Correct return value on signed long int overflow/underflow.
Correct handling of "0x" prefix not followed by a hex digit.

JIRA: RTOS-869
@Darchiv Darchiv merged commit 61f9d17 into master Nov 6, 2024
36 checks passed
@Darchiv Darchiv deleted the jmaksymowicz/RTOS-869 branch November 6, 2024 21:05
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.

strto functions don't set an errno in case of a failure
2 participants