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

Preview of changes, for discussion #56

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ledvinap
Copy link

This is work in progress, needs BIG cleanup. It's here only for discussion

Rough overview of changes:

  • use struct printf_state to pass around % specifier info.
    Mostly it seems to work fine and reduces register pressure, but sometimes it's a bit cumbersome. I'll look into it further.

  • use struct out_base to handle out callback.
    Personally I prefer this way of handling callbacks - parameters can be easily added, out_base can be included in any other object (for example serial driver struct) etc.
    In case of out function it is a bit overkill - it is undesirable to include printf_state in out_base (a lot of compiler optimizations would be disabled) and it is possible to store both function pointer and argument in state.

  • change idx to be uintptr_t and allow starting from any value. idx is incremented on each output, so it is desirable to have it in register. With uintptr_t it is possible to store output buffer pointer directly, saving some instruction, with almost zero penalty (for example it is impossible to write last byte of memory)

  • test idx < maxidx before output function call
    better caching of values in register, simpler output functions

  • add FLAGS_ZERO
    I hope it's consistent with rest of flags

  • unify _ntoa_long and _ntoa_long_long using macro (only parameter type differs)
    Removes code duplication at cost of more complicated debugging

  • implement (almost?) correct rounding for %f
    fmsub is relatively simple, some FPUs (Cortex-M4) support equivalent instruction

  • fix %g trailing zeroes
    The implementation isn't as elegant as I'd like, it may be conditional or maybe there is better way to rewrite it

  • I encountered interesting unspecified case:
    (unsigned int)-INT_MIN (and (unsigned int)(0 - value) in current code) is causing integer overflow, which is unspecified behavior in C standard. gcc was able to use this to sign-extend long first, producing incorrect result for printf("%d", INT_MIN);

  • move zero-termination from _vsnprintf to snprintf
    fixes printf("%c", 0);, is cleaner (only string needs termination), simplifies code a little

  • uintptr_t idx removes need to special-case sprintf(NULL, ...) case, just pass 0 as limit

  • added new test cases from other sources (mainly libsanity), discovering some bugs (mostly opened as issues).
    modified tests are just temporary hack to get testing quickly, I'll clean it up

@mpaland
Copy link
Owner

mpaland commented Apr 29, 2019

Hello @ledvinap ,
this is really a bunch of bugs you found.
I'll review them and create a next version branch to get your updates merged in.
Thanks a lot for your outstanding work!

@mpaland mpaland self-requested a review April 29, 2019 15:07
@mpaland mpaland self-assigned this Apr 29, 2019
@ledvinap
Copy link
Author

@mpaland : Hi!
Most of bugs are from libinsanity tests. Mostly easy to fix. The rest of the code needs some polishing, still thinking about how to do it correctly.

I'll prepare some parts as separate PRs, fixing bugs and sometimes improving existing code. Then it can be decided about more philosophical changes ...

Can you please overview list of changes and do a quick comment about them (OK/don't like/not sure/needs further work)?

@legier
Copy link

legier commented Nov 6, 2019

When I can expect to pull these commits into main branch and have new release?

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.

3 participants