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

minimal-printf: fix display decimals between -1 to 0 incorrect issue. #13784

Closed
wants to merge 5 commits into from

Conversation

Linguanghsou
Copy link

@Linguanghsou Linguanghsou commented Oct 19, 2020

Summary of changes

Fixes "minimal-printf doesn't display decimals between -1 to 0 correctly" listed in #13783

Impact of changes

Migration actions required

Documentation


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

double d = -0.00139;
printf("d = %f\r\n",d);

--->
d = -0.001391


Reviewers


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Oct 19, 2020
@ciarmcom
Copy link
Member

@Linguanghsou, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from a team October 19, 2020 11:30
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Can you add details to the commit message? How is this fixing the issue, I am interested in how the casting here (changing argument type and later casting it) works

@Linguanghsou
Copy link
Author

Linguanghsou commented Oct 20, 2020

Can you add details to the commit message? How is this fixing the issue, I am interested in how the casting here (changing argument type and later casting it) works

@ciarmcom

static void mbed_minimal_formatted_string_signed(char *buffer, size_t length, int *result, MBED_SIGNED_STORAGE value, FILE *stream)
{
    MBED_UNSIGNED_STORAGE new_value = 0;

    /* if value is negative print sign and treat as positive number */
    if (value < 0) {
        /* write sign */
        mbed_minimal_putchar(buffer, length, result, '-', stream);

        /* get absolute value using two's complement */
        new_value = ~((MBED_UNSIGNED_STORAGE) value) + 1;
    } else {
        new_value = value;
    }

In original mbed_minimal_formatted_string_signed() above, the type of agument value is a signed integer. Given a double value -0.00139 being printed, its integer part is 0 (-0 is equal to 0), so the condition "if (value < 0)" is false and it was handled as a positive decimal like 0.00139.

In modification listed below, the value is a double type. Given a double value -0.00139, the condition "if (value < (double)0)" is true and it is handled as a negative decimal as expected.

static void mbed_minimal_formatted_string_signed(char *buffer, size_t length, int *result, double value, FILE *stream)
{
    MBED_UNSIGNED_STORAGE new_value = 0;
    MBED_SIGNED_STORAGE integer = value;

    /* if value is negative print sign and treat as positive number */
    if (value < (double)0) {
        /* write sign */
        mbed_minimal_putchar(buffer, length, result, '-', stream);

        /* get absolute value using two's complement */
        new_value = ~((MBED_UNSIGNED_STORAGE) integer) + 1;
    } else {
        new_value = integer;
    }

    /* use unsigned long int function */
    mbed_minimal_formatted_string_unsigned(buffer, length, result, new_value, stream);
}

@Linguanghsou Linguanghsou requested review from 0xc0170 and removed request for a team October 21, 2020 03:33
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 23, 2020

Thanks for the explanation, can you amend the commit message as requested?

@0xc0170 0xc0170 requested a review from a team October 23, 2020 14:13
@Linguanghsou Linguanghsou changed the title minimal-printf: fix display decimals between -1 to 0 incorrect issue. minimal-printf: fix display decimals between -1 to 0 incorrect issue. In original mbed_minimal_formatted_string_signed(), the type of augment value is a signed integer. Given a double value -0.00139 being printed, its integer part is 0 (-0 is equal to 0), so the condition "if (value < 0)" is false and it was handled as a positive decimal like 0.00139. In the fix, the value is a double type. Given a double value -0.00139, the condition "if (value < (double)0)" is true and it is handled as a negative decimal as expected. Oct 26, 2020
…In original mbed_minimal_formatted_string_signed(), the type of augment value is a signed integer. Given a double value -0.00139 being printed, its integer part is 0 (-0 is equal to 0), so the condition "if (value < 0)" is false and it was handled as a positive decimal like 0.00139. In the fix, the value is a double type. Given a double value -0.00139, the condition "if (value < (double)0)" is true and it is handled as a negative decimal as expected.
@Linguanghsou Linguanghsou changed the title minimal-printf: fix display decimals between -1 to 0 incorrect issue. In original mbed_minimal_formatted_string_signed(), the type of augment value is a signed integer. Given a double value -0.00139 being printed, its integer part is 0 (-0 is equal to 0), so the condition "if (value < 0)" is false and it was handled as a positive decimal like 0.00139. In the fix, the value is a double type. Given a double value -0.00139, the condition "if (value < (double)0)" is true and it is handled as a negative decimal as expected. minimal-printf: fix display decimals between -1 to 0 incorrect issue. Oct 26, 2020
@mergify mergify bot dismissed 0xc0170’s stale review October 26, 2020 03:45

Pull request has been modified.

@Linguanghsou
Copy link
Author

Thanks for the explanation, can you amend the commit message as requested?

Done, please check the commit message.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 26, 2020

@Linguanghsou I can see two commits and one merge commit, can you rebase to squash all three into one ?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 4, 2020

There was a different suggestion to fix this: #13783 (comment) . isn't this better than what is proposed here?

…In original mbed_minimal_formatted_string_signed(), the type of augment value is a signed integer. Given a double value -0.00139 being printed, its integer part is 0 (-0 is equal to 0), so the condition "if (value < 0)" is false and it was handled as a positive decimal like 0.00139. In the fix, the value is a double type. Given a double value -0.00139, the condition "if (value < (double)0)" is true and it is handled as a negative decimal as expected.
@Linguanghsou Linguanghsou reopened this Nov 5, 2020
@mergify mergify bot removed needs: work release-type: patch Indentifies a PR as containing just a patch labels Nov 5, 2020
@Linguanghsou
Copy link
Author

@Linguanghsou I can see two commits and one merge commit, can you rebase to squash all three into one ?

@0xc0170 Please noted that I created #13865 to replace current PR to have only one commit message.
#13865

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