Skip to content

Conversation

@PiotrZierhoffer
Copy link
Contributor

This PR unifies the implementation of the conversion mechanism from string to integer type.

It also adds two missing functions: strtoll and strtoull.

@zephyrbot
Copy link

zephyrbot commented Aug 22, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

They differ only in details.

I elevated the basic "working" type to unsigned long long to handle
larger types with the same function.

Signed-off-by: Piotr Zierhoffer <[email protected]>
From IEEE Std 1003.1-2017:

These functions shall convert the initial portion of the string pointed
to by str to a type unsigned long and unsigned long long
representation, respectively.

Signed-off-by: Piotr Zierhoffer <[email protected]>
@PiotrZierhoffer PiotrZierhoffer force-pushed the strtoll_functions branch 2 times, most recently from 989daa1 to 55be7d8 Compare August 22, 2019 14:32
They are no longer needed in the sample shim layer, as they are
available in minimal-libc.

Signed-off-by: Piotr Zierhoffer <[email protected]>
@pfalcon
Copy link
Contributor

pfalcon commented Aug 23, 2019

Closing/reopening to restart CI.

@pfalcon pfalcon closed this Aug 23, 2019
@pfalcon pfalcon reopened this Aug 23, 2019
@pfalcon
Copy link
Contributor

pfalcon commented Aug 23, 2019

I'm +1 for further elaborating minlibc, as I said on multiple occasions, and it's especially important as we're going to bloat up newlib: zephyrproject-rtos/sdk-ng#62

But to vote this up with clear conscience, code size change stats would rather be provided.

@pfalcon
Copy link
Contributor

pfalcon commented Aug 25, 2019

But to vote this up with clear conscience, code size change stats would rather be provided.

For example, stats on enabling long long support for newlib stdio: zephyrproject-rtos/sdk-ng#101 (comment) . +13K or 100% code size growth for a simple app. (Yeah, that accounts for disabling "nano formatting" support, not just switching all existing calculations in print/scanf from long to long long).

@PiotrZierhoffer
Copy link
Contributor Author

Thanks @pfalcon , I will try to provide some results when I'm at my PC

@PiotrZierhoffer
Copy link
Contributor Author

@pfalcon I am not 100% sure if my test is ok or not, please advise.

I took samples/hello_world and added strtoul call (a function that was already there) and compiled it for qemu_cortex_m3 with my HEAD and the commit I branched out from.

My PR:

   text	   data	    bss	    dec	    hex	filename
   9834	    760	   3924	  14518	   38b6	zephyr/zephyr.elf

Test on d157527 (merge base):

   text	   data	    bss	    dec	    hex	filename
   8918	    760	   3924	  13602	   3522	zephyr/zephyr.elf

There is some increase in the size, but it's not up to me to decide if it's acceptable or not.

Just to be clear, I don't have any work relying on this PR, just had it prepared earlier and thought it could be useful regardless of our civetweb PRs.

@pfalcon pfalcon requested a review from mike-scott August 29, 2019 09:23
@pfalcon
Copy link
Contributor

pfalcon commented Aug 29, 2019

There is some increase in the size, but it's not up to me to decide if it's acceptable or not.

Yes, 916 for ARM Thumb2 specifically. Let me also skip IMHOish commentary whether it's much or not (it's definitely not too much). The point is however that we should control the codesize of minlibc, e.g. performs code size comparisons like you did. As it's done, this patch should be clear from this PoV.

And we definitely should keep growing functionality of minlibc, and should encourage people who do this (in sustainable manner, per above). So, 2 more reasons to vote this up beyond pure usefulness, so let me do just that.

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

This is useful addition. For many real-world scenarios, 32-bit are not enough. And as we add networking into mix, bigger-than-32-bit values need to be exchanges using various protocols, many of which are text based. There should be a way to deal with these for cases when full libc (like newlib) is too much.

@mike-scott mike-scott removed their request for review November 4, 2019 22:08
Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

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

code looks fine but I would like this patch a lot more if there were some tests provided to show that these newly added APIs work.

@PiotrZierhoffer
Copy link
Contributor Author

PiotrZierhoffer commented Nov 6, 2019

@andrewboie would tests/lib/c_lib be a good place to put those tests?

@jhedberg
Copy link
Member

jhedberg commented Jan 7, 2020

@andrewboie would tests/lib/c_lib be a good place to put those tests?

@PiotrZierhoffer that looks like a good place to me, unless @andrewboie has some better suggestion

@galak
Copy link
Contributor

galak commented Jan 29, 2020

@PiotrZierhoffer any plans to add tests?

@PiotrZierhoffer
Copy link
Contributor Author

It went off my radar, but I'll get back to it, hopefully next week

@mglettig
Copy link

@PiotrZierhoffer Do you have any update on strtoll and stroull support for minimal libc? Due to the usage of Civetweb I'm stuck with minimal libc and can't use new libc. But now I'm using a lib from Nordic that needs strtoll and stroull. See here https://devzone.nordicsemi.com/f/nordic-q-a/76717/nordic-nrf-sdk-v1-6-issue-with-at_cmd_parser/317127#317127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants