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

More refactoring to completely (?) mimic previous library behavior for scanning contents of the file #11

Merged
merged 4 commits into from
Dec 16, 2013

Conversation

qkoziol
Copy link
Contributor

@qkoziol qkoziol commented Dec 16, 2013

Switch test for nc_inq_unlimdim and nc_inq_unlimdims to use the same ordering as the rest of the dimension queries. Correct error in library where types used in sub-group variables but that were added to the file after the sub-group was created weren't available for sub-group variables to use. Start cleaning up test suite and un-commenting tests that were commented out (got up to nc_test4/tst_fills2.c, alphabetically) before running into an error in HDF5.

…ecial()

    to clean up resources properly on failure.

Refactored doubly-linked list code for objects in the libsrc4 directory,
    cleaning up the add/del routines, breaking out the common next/prev
    pointers into a struct and extracting the add/del operations on them,
    changed the list of dims to add new dims in the same order as the other
    types, made all add routines able to optionally return a pointer to the
    newly created object.

Removed some dead code (pg_var(), nc4_pg_var1(), nc4_pg_varm(), misc. small
    routines, etc)

Fixed fill value handling for string types in nc4_get_vara().

Changed many malloc()+strcpy() pairs into calls to strdup().

Cleaned up misc. other minor Coverity issues.
…ordering as

the rest of the dimension queries.  Correct error in library where types used
in sub-group variables but that were added to the file after the sub-group was
created weren't available for sub-group variables to use.  Start cleaning up
test suite and un-commenting tests that were commented out (got up to
nc_test4/tst_fills2.c, alphabetically) before running into an error in HDF5.
@WardF
Copy link
Member

WardF commented Dec 16, 2013

Seeing an issue on 32-bit Windows builds in nc_test4/tst_converts2.c. The issue is recorded at the CDash site here:

http://my.cdash.org/testDetails.php?test=13241998&build=551669

I can fix this easily enough for 32-bit Windows, but just wanted to make a note of it.

@qkoziol
Copy link
Contributor Author

qkoziol commented Dec 16, 2013

On Dec 16, 2013, at 12:13 PM, Ward Fisher notifications@github.com wrote:

Seeing an issue on 32-bit Windows builds in nc_test4/tst_converts2.c. The issue is recorded at the CDash site here:

http://my.cdash.org/testDetails.php?test=13241998&build=551669

I can fix this easily enough for 32-bit Windows, but just wanted to make a note of it.

Yes, I'll take a look at it.  (I enabled some commented out tests there, and maybe they need some extra attention)

    Quincey

@WardF
Copy link
Member

WardF commented Dec 16, 2013

Sounds good; everything else looks fine re: our test platforms. Once Russ has made his review I'll merge it into the trunk.

@WardF WardF merged commit 0d42ac7 into Unidata:master Dec 16, 2013
@WardF
Copy link
Member

WardF commented Dec 16, 2013

I've merged into the trunk; in addition to the windows issue mentioned above, we're seeing the same test failure on sol and my 32-bit Linux system, but this should be easily addressable.

@qkoziol
Copy link
Contributor Author

qkoziol commented Dec 28, 2013

Looks like you've checked in a fix that's working for this, so I'm going to go on to the new Coverity issues. (And then back to working my way through the tests)

On Dec 16, 2013, at 4:50 PM, Ward Fisher notifications@github.com wrote:

I've merged into the trunk; in addition to the windows issue mentioned above, we're seeing the same test failure on sol and my 32-bit Linux system, but this should be easily addressable.


Reply to this email directly or view it on GitHub.

@WardF
Copy link
Member

WardF commented Dec 28, 2013

On 12/28/13, 12:23 PM, Quincey Koziol wrote:

Looks like you've checked in a fix that's working for this, so I'm
going to go on to the new Coverity issues. (And then back to working
my way through the tests)

On Dec 16, 2013, at 4:50 PM, Ward Fisher notifications@github.com
wrote:

I've merged into the trunk; in addition to the windows issue
mentioned above, we're seeing the same test failure on sol and my
32-bit Linux system, but this should be easily addressable.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#11 (comment).

Yup; didn't mean to step on your toes, but the fix was pretty easy and
the one red 'test failing' on the CDash dashboard was really bugging me ;).

-Ward

@qkoziol
Copy link
Contributor Author

qkoziol commented Dec 28, 2013

On Dec 28, 2013, at 3:51 PM, Ward Fisher notifications@github.com wrote:

On 12/28/13, 12:23 PM, Quincey Koziol wrote:

Looks like you've checked in a fix that's working for this, so I'm
going to go on to the new Coverity issues. (And then back to working
my way through the tests)

On Dec 16, 2013, at 4:50 PM, Ward Fisher notifications@github.com
wrote:

I've merged into the trunk; in addition to the windows issue
mentioned above, we're seeing the same test failure on sol and my
32-bit Linux system, but this should be easily addressable.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#11 (comment).

Yup; didn't mean to step on your toes, but the fix was pretty easy and
the one red 'test failing' on the CDash dashboard was really bugging me ;).

No problem at all, thanks for jumping in and taking care of it!  :-)  I was just confirming that you had handled it and I was going on to the next set of issues.

Quincey

edhartnett added a commit to NetCDF-World-Domination-Council/netcdf-c that referenced this pull request Feb 2, 2018
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.

2 participants