Skip to content

libc: minimal: Add strtoll() and strtoull() #39612

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

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

MeisterBob
Copy link
Contributor

@MeisterBob MeisterBob commented Oct 21, 2021

strtoll() and strtoull() are copies of strtol() and strtoul() with types
changed to long long instead of long.

Signed-off-by: Gerhard Jörges [email protected]

Closes #36645

tejlmand
tejlmand previously approved these changes Nov 8, 2021
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

  1. Please add the tests for these functions in tests/lib/c_lib.
  2. Need to update the relevant documentation:
    https://github.com/zephyrproject-rtos/zephyr/blame/main/doc/guides/portability/posix.rst#L311
  3. Need to remove the local strtoll implementation:
    long long strtoll(const char *str, char **endptr, int base)

    long long strtoll(const char *str, char **endptr, int base);

@MeisterBob
Copy link
Contributor Author

I was busy in an other project the last weeks, but finally I added tests and documentation and removed stubs from civetweb samples. Also rebased the branch..

@MeisterBob
Copy link
Contributor Author

I stumbled across #16511 by @PiotrZierhoffer. I liek the way he unified the conversion mechanism. The only thing missing in his PR where tests. I would suggest to take the tests I implemented and put them into his PR.


char border1[] = "+18446744073709551615";
char border2[] = "-18446744073709551615000";
char border3[] = "+18446744073709551619";
Copy link

Choose a reason for hiding this comment

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

Suggest adding a couple more cases here to test a long string of digits but not a large number, and a simply very large number:

border[] = "0x0000000000000000000000000000000000001"; // should pass
border[] = "10000000000000000000000000000000000001"; // should fail (too big)
border[] = "-10000000000000000000000000000000000001"; // should fail (too big)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added those cases to strtoll and strtoull test.

- strtoll() and strtoull() are copies of strtol() and strtoul() with
  types changed to long long instead of long.
- added tests
- added documentation
- removed stubs from civetweb sample

Signed-off-by: Gerhard Jörges <[email protected]>
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm - reapprove

@stephanosio stephanosio added the Licensing The PR has licensing issues => licensing expert to review label Feb 22, 2022
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

This PR contains the source files that are BSD-4-Clause-UC licensed.

This is different to the usual 3-clause BSD license variants that we have previously accepted into the codebase; in particular, the third clause is likely going to be a problem for the project.

 * 3. All advertising materials mentioning features or use of this software
 *    must display the following acknowledgment:
 *	This product includes software developed by the University of
 *	California, Berkeley and its contributors.

If we are to accept this as is, this needs to go through the board as per the Contribution Guidelines.

The most practical solution would be to replace the strto(u)ll implementation with the one that is more freely licensed (e.g. BSD-2-Clause, BSD-3-Clause, ...).

cc @nashif @carlescufi

@MeisterBob
Copy link
Contributor Author

It's the same license strtol and strtoul are using. If you can point out an implementation that more suitable licensed I would be happy to replace strtol, strtoul, strtoll and strtoull.

@stephanosio
Copy link
Member

It's the same license strtol and strtoul are using. If you can point out an implementation that more suitable licensed I would be happy to replace strtol, strtoul, strtoll and strtoull.

@MeisterBob You are right. They are indeed BSD-4-Clause-UC (not sure why these did not turn up while grepping ...).

In that case, I assume this license has already been approved by the board. I would like @nashif to give a confirmation though, since those date back to the beginning of the project (maybe before such rules were in place?) ...

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

I am lookin at adding a few more of those. Can you please move those to a single source file and combine the use of the whitespace trimming so that it is not duplicated for every function?

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

I am lookin at adding a few more of those. Can you please move those to a single source file and combine the use of the whitespace trimming so that it is not duplicated for every function?

Please ignore this, I will take care of this in a follow-up PR.

@de-nordic
Copy link
Collaborator

Shouldn't this addition be included into release notes?

@stephanosio stephanosio added the Release Notes To be mentioned in the release notes label Mar 18, 2022
@carlescufi
Copy link
Member

This replaces #43289 for the mesh code, with further additions that I will provide.

@carlescufi
Copy link
Member

Shouldn't this addition be included into release notes?

Release notes are populated at release time generally, except for API breakage/changes.

@nashif nashif added the TSC Topics that need TSC discussion label Mar 18, 2022
@carlescufi
Copy link
Member

@MeisterBob could you please create an issue as described in the doc to get the inclusion of external code accepted please?

@nashif nashif removed the TSC Topics that need TSC discussion label Mar 23, 2022
@carlescufi
Copy link
Member

Approved by the TSC, merging with failing checks:

  • Coding guidelines: Use of reserved identifiers (this is the C library)
  • Scancode: Use of a non-Apache license (approved by the TSC)

@carlescufi carlescufi merged commit 4fd24a4 into zephyrproject-rtos:main Mar 24, 2022
@kestewart
Copy link
Member

This is a non-OSI approved license. Per:https://docs.zephyrproject.org/latest/contribute/external.html#external-src-process it's going to need board review.

@MeisterBob MeisterBob deleted the strtoll branch April 19, 2022 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: C Library C Standard Library area: Documentation area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test Licensing The PR has licensing issues => licensing expert to review Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minimal libc: add strtoll and strtoull functions
9 participants