Two minor endian-adjacent fixes

[neuschaefer]

see individual commits

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ WardF
❌ neuschaefer
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[neuschaefer]

Regarding the CLA: This is a trivial contribution, I wish to proceed without CLA.

[WardF]

Regarding the CLA: This is a trivial contribution, I wish to proceed without CLA.

I appreciate your willingness to do this, although I'll need to check to see what sort of flexibility we have, institutionally. @dopplershift?

@neuschaefer If we are not permitted to skip the CLA (this decision is out of my hands), I will replicate this manually. Thank you!

[WardF]

After taking a further look, this appears to break the test; after reading how the test is performed, I believe that the original form is correct. @neuschaefer did this work for you? And if so, what platform did you test it on? Thanks!

[dopplershift]

Sure, this is trivial enough to go without CLA.

[neuschaefer]

@neuschaefer did this work for you? And if so, what platform did you test it on? Thanks!

Admittedly, I did not test this on a little-endian machine (where the tests previously passed). I did that now, with the following results:

• With HDF5, tst_h5_endians does indeed fail the Read/Write Big-Endian Int test
• The test named test_endians passes in builds with and without HDF5

After taking a further look, this appears to break the test; after reading how the test is performed, I believe that the original form is correct.

I disagree. Just from the shape of the code alone, It looks a lot like a copy/paste error:

        printf("\tBig-Endian Int...\t");
        if ((retval = nc_put_var(ncid,be_int_varid,idata_in)))
            return retval;
        if ((retval = nc_get_var(ncid,be_int_varid,idata_be_out)))
//          return retval;     // this line inserted by me
        for(failed=0,i=0;i<NDIM;i++) {if(idata_in[i] != idata_be_out[i]) {printf("failed\n"); failures++; failed++; break;}}
        if(!failed) printf("passed\n");

The logic, as it was before my patch, amounts to the following:

• if nc_get_var failed (return value != 0 NO_NOERR)
  • then validate that the variable it (successfully) retrieved has the expected value (through the for loop)

That the if should apply to the following for is indicated neither through indentation, nor through curly braces, nor through a comment, and it's a deviation from an established pattern of if (retval = nc_get_var(...)) return retval; in the same file and function.

With the return retval; added, the if body is properly indented, follows the same pattern as in other places, and makes sense.

I think what happened, instead, is that my fix uncovered a bug in the library, probably the same bug that has been visible on big-endian systems. Because nc_get_var usually doesn't fail, the test never attempted to validate idata_be_out.

[WardF]

Taking a look to reconcile the logic and the failing tests.

[edhartnett]

These are good changes. Thanks!

[edhartnett]

Failing tests hopefully will be fixed by recent changes plus #3273