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

GSoC 2016: Correct real write bug #3870

Merged
merged 3 commits into from May 18, 2016
Merged

GSoC 2016: Correct real write bug #3870

merged 3 commits into from May 18, 2016

Conversation

ghost
Copy link

@ghost ghost commented May 13, 2016

This pull request tries to correct the bug noticed in #2148. It will switch to an exponential notation if the standard precision isn't enough to print the decimal part of the number.

Passed test/io and test/release on both linux64 and darwin with default settings.

@bradcray
Copy link
Member

@mppf seems like the natural reviewer here, if he hasn't been made aware of this one already.

@ben-albrecht
Copy link
Member

ben-albrecht commented May 13, 2016

Hi @Panzone - nice work on this change.

I ran the full test suite for you and found the following errors:

 [Error matching program output for modules/standard/math/acosh]
 [Error matching program output for modules/standard/memory/countMemory/countMemory]
 [Error matching program output for types/scalar/bradc/piVals]
 [Error matching program output for types/scalar/bradc/piVals1a]
 [Error matching program output for types/scalar/bradc/piVals2]
 [Error matching program output for users/shetag/tensor]

I can send outputs if you're unable to reproduce them locally.

@mppf
Copy link
Member

mppf commented May 16, 2016

The code change itself looks good to me. Those failing tests probably just need .good updates but you'll need to double-check that the new output makes sense. Also, I'd like to see a new test that verifies some integers of different sizes print as expected (varying the number of digits in the integer).

@ghost
Copy link
Author

ghost commented May 17, 2016

Corrected the failing tests and added a new one.

d = 1000.0
e = 10000.0
f = 1.00000e+05
g = 1e+06
Copy link
Member

Choose a reason for hiding this comment

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

Why does f print out with 1.00000e+05 and not 1+05 ? Can you get it to print the simpler version?

@mppf
Copy link
Member

mppf commented May 17, 2016

It looks good to me, other than the issue I noted. In particular, using the default format (e.g. with writeln) I don't expect it to print digits after the trailing zeros in exponential notation. Still, we're close...

@ghost
Copy link
Author

ghost commented May 18, 2016

It should now print the simpler version.

This code

var pi = 314159.2654;
writeln(pi);

pi = 100000.0;
writeln(pi);

pi = 100300.9;
writeln(pi);

now produces

3.14159e+05
1e+05
1.00301e+05

@mppf
Copy link
Member

mppf commented May 18, 2016

Passed full local testing.

@mppf mppf merged commit 96428e1 into chapel-lang:master May 18, 2016
@ghost ghost deleted the correct-real-write branch May 19, 2016 07:21
awallace-cray added a commit that referenced this pull request Jun 9, 2016
Correct real write bug (from #3870)
[cherry-pick]
@mppf mppf changed the title Correct real write bug GSoC 2016: Correct real write bug Aug 24, 2016
mppf added a commit that referenced this pull request Feb 16, 2021
fix I/O bug in _ftoa_core to solve issue 17170

Resolves #17170

PR #3870 added some code to `_ftoa_core` that examines and updates the
output. But, it did not correctly handle the case that not all of the
output was present (in particular when there was not enough room left in
the current buffer) or when `snprintf` returned an error. This PR updates
the existing conditional to check that the `snprintf` was successful.
(Note that if `snprintf` returned `buf_sz` that would actually indicate
not enough room in the output because `snprintf` would store the trailing
`\0` byte instead of the expected output as the last character).

Adds a test that reproduces the error so we can avoid it in the future.

- [x] full local testing

Reviewed by @ben-albrecht - thanks!
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.

None yet

3 participants