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.

Sign up for GitHub

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

Jump to bottom

Issue 159 #180

Merged
merged 43 commits into from
Dec 30, 2015
Merged

Issue 159 #180

merged 43 commits into from
Dec 30, 2015

Conversation

WardF
Copy link
Member

@WardF WardF commented Dec 28, 2015

Addressing two issues on ARM, merging via pull request so that @dmh has a chance to review it. Issues were originally reported by @opoplawski at issue #159.

  1. char is unsigned by default. This fouled some of our tests.
  2. Converting double to unsigned int (which throws an NC_ERANGE error under normal circumstances but is still tested for and technically permitted) is undefined behavior.

The former issue has been addressed, and the second issue (while still undefined) now results in behavior consistent with what's observed on Intel platforms.

WardF added 30 commits December 22, 2015 11:39
@WardF
Added a test case for bug.
9d4e72e
@WardF
Added a check for signed char.
2ea988f
@WardF
Fixed range error for signed char test, now we have a range error for…
f41058c 
… nc_get_var_text.
@WardF
Added an lldb debugging file.
c39ea84
@WardF
Fixed text-related range error, now observing value read not expected.
c0e32dd
@WardF
nc_test2 is now fixed, but we are seeing issues related to text attri…
b40f10e 
…butes in nc_test, when -funsigned-char is set in CFLAGS.
@WardF
Fixed an implicit cast in nc2dbl that caused problems when char was u…
60fc0f1 
…nsigned by default.
@WardF
Addressed an issue in check_vars when -funsigned-chars
92d374c
@WardF
Fixed unsigned char issue in check_atts.
c6fd79d
@WardF
Fixed an unsigned-char related issue in test_nc_rename_att.
b66e377
@WardF
Added a small test program to print out characteristics of chars.
5505187
@WardF
Fixed a dangling issue with writing text.
4055a81
@WardF
Added a test derived from nc_test2 cdf5 test data, for github issue h…
52f0fc8 
…ttp://github.com/Unidata/netcdf-c/issues/159
@WardF
Tweaked new test.
5cb4efe
@WardF
Duplicating the issue we've observed by forcing an overflow error.
7bf5012
@WardF
Additional tweaking of the test.
a0de63b
@WardF
I think we have this recreated as we would expect.
f83ec50
@WardF
Closer to replicating I think.
4d9cb91
@WardF
Captured a conversion error.
bd6f490
@WardF
More tweaks during debugging.
8e3c7fe
@WardF
Tweaking the test a little bit
3e2a1cc
@WardF
Issue identified, see #159 for specifics. Also see http://stackoverfl…
d10688a 
…ow.com/questions/10541200/is-the-behaviour-of-casting-a-negative-double-to-unsigned-int-defined-in-the-c-s for some background info that lead me to the fix.
@WardF
Added explit cast from signed in ncx.m4 to address #159. It is likely…
6eec405 
… overly broad; I will refine it once I've had a chance to read up on m4.
@WardF
Some additional errors reared up after fixing the main frustrating on…
925da14 
…e. Working towards fixing them.
@WardF
Fenceposting the bug fixes with __arm__ ifdefs.
d71e9ed
@WardF
More architecture-specific fenceposting.
103863c
@WardF
Updated travis to test both signed and unsigned chars.
fce2f87
@WardF
Updated travis.yml
9e5c03b
@WardF
Putting travis configuration back the way it was for now.
30d2b09
@WardF
Added more debugging info to gdb commands file.
7a417b9
@WardF WardF added this to the 4.4.0 milestone Dec 28, 2015
@WardF WardF self-assigned this Dec 28, 2015
@WardF
Copy link
Member Author

WardF commented Dec 29, 2015

Many of the places where I use __arm__ arm instead of __UNSIGNED_CHAR__ are because the tests don't fail on intel-based platforms regardless of whether the char is signed or not. I'm still up in the air on this, it feels like it should be __UNSIGNED_CHAR__ but functionally, __arm__ is more correct.

@WardF
Copy link
Member Author

WardF commented Dec 30, 2015

Ok, I've also updated our travis-ci configuration to run tests both with -fsigned-char and -funsigned-char to hopefully catch this sort of unexpected issue in the future. I'm going to go ahead and merge, but if this ends up being overly broad we can address that.

@WardF WardF merged commit 09ab3fc into master Dec 30, 2015
@WardF WardF mentioned this pull request Dec 30, 2015
Closed
@WardF WardF deleted the unsigned-char-gh159 branch January 8, 2016 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@WardF WardF

Labels
type/enhancement
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.
1 participant
@WardF