Assume C99 fixed sized ints exist, use them #470

seanm · 2021-03-13T22:33:00Z

No description provided.

seanm · 2021-03-13T22:33:39Z

@qkoziol @derobins is this an acceptable direction? I can do the same for 8, 16, and 64.

qkoziol · 2021-03-13T22:44:43Z

The standard seems to indicate that these are not required by a compiler, which is why our previous coding provided workarounds when they weren't available. It's possible that we'd like to require them for HDF5 to work correctly now, but I don't know if there's any situations where that might be a problem for any users.

derobins

Let's do the H5public.h changes in a separate PR so we can merge the internal changes to 1.12 and 1.10.

derobins · 2021-03-14T06:11:29Z

We directly use C99 types like uint64_t, uint32_t, etc. in many places in HDF5. If even Microsoft supports it, I think it's safe to assume it's ubiquitous. I'm definitely in favor of reducing our ifdef madness by eliminating obsolete cruft.

seanm · 2021-03-14T17:49:43Z

Let's do the H5public.h changes in a separate PR so we can merge the internal changes to 1.12 and 1.10.

In this case though, line 293 is always true, is it not? The whole chunk is dead code.

qkoziol · 2021-03-15T16:47:29Z

We directly use C99 types like uint64_t, uint32_t, etc. in many places in HDF5. If even Microsoft supports it, I think it's safe to assume it's ubiquitous. I'm definitely in favor of reducing our ifdef madness by eliminating obsolete cruft.

We use it because we typedef it when it's not defined by the compiler.

I do think it's OK to get rid of our backward compatibility shims at this point though. :-)

qkoziol

As long as this passes for all CI & nightly regression tests, should be good.

epourmal · 2021-03-15T16:50:07Z

Yes, let’s do it! Thank you, all! On Mar 15, 2021, at 11:47 AM, Quincey Koziol ***@***.***> wrote: We directly use C99 types like uint64_t, uint32_t, etc. in many places in HDF5. If even Microsoft supports it, I think it's safe to assume it's ubiquitous. I'm definitely in favor of reducing our ifdef madness by eliminating obsolete cruft. We use it because we typedef it when it's not defined by the compiler. I do think it's OK to get rid of our backward compatibility shims at this point though. :-) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#470 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADLFT3OVW5UJHLZ4QBRL5FTTDY23LANCNFSM4ZEI2GUQ>.

seanm · 2021-03-15T17:34:02Z

OK, I'll keep working on this PR then...

byrnHDF

This define, H5_SIZEOF_LONG_DOUBLE, cannot be removed anywhere because not all platforms support long double. Windows insists that long double is just double and this define will be zero on that platform.

seanm · 2021-03-16T14:38:26Z

This define, H5_SIZEOF_LONG_DOUBLE, cannot be removed anywhere because not all platforms support long double. Windows insists that long double is just double...

long double has been in C since C89. That's not to say that it needs to be any bigger than double, just as long int is not necessarily any bigger than int.

and this define will be zero on that platform

Really?! It's not 8, just like H5_SIZEOF_DOUBLE? The comment above it says:

/* The size of `long double', as computed by sizeof. */
#cmakedefine H5_SIZEOF_LONG_DOUBLE @H5_SIZEOF_LONG_DOUBLE@

And MS docs say sizeof(long double) is 8.

byrnHDF · 2021-03-17T12:25:35Z

Really?! It's not 8, just like H5_SIZEOF_DOUBLE? The comment above it

It is 0 because the code it encloses would be used, in tools lib the else code of the same if define is used.

byrnHDF · 2021-03-17T12:52:00Z

Really?! It's not 8, just like H5_SIZEOF_DOUBLE? The comment above it

It is 0 because the code it encloses would be used, in tools lib the else code of the same if define is used.

I just double-checked that code and realized I might be wrong, the code in the ifdef block might be included and the "if type equals" fail the result will be the same that I see.

byrnHDF

Need to check a windows build, to verify the define is not 0.

brtnfld

Fortran-related changes look good.

seanm · 2021-03-17T16:42:34Z

I just double-checked that code and realized I might be wrong, the code in the ifdef block might be included and the "if type equals" fail the result will be the same that I see.

Could you point me to the code to which you refer?

byrnHDF · 2021-03-17T18:35:19Z

I just double-checked that code and realized I might be wrong, the code in the ifdef block might be included and the "if type equals" fail the result will be the same that I see.

Could you point me to the code to which you refer?

tools/lib/h5tools_dump.c change.

src/H5Ztrans.c

lrknox · 2021-03-19T18:57:11Z

This is failing a test in dtransform.c on Ubuntu with gcc 9.3.0, both before and after the latest commit:

Testing contiguous, no data type conversion (ldouble->ldouble)

It seems the calculated value for the first number in the array is "nan" instead of "2.22222".

seanm · 2021-03-22T15:34:25Z

@lrknox why the new commit f4a044b? Is it to undo a799020? I separated a799020 as @derobins requested, and can drop that commit when this is no longer WIP.

But I still don't see the need. When can #if H5_SIZEOF_INT64_T >= 8 be false? You realize all that code only does anything in such dead branches, right?

lrknox · 2021-03-22T16:02:48Z

Let's do the H5public.h changes in a separate PR so we can merge the internal changes to 1.12 and 1.10.

@derobins H5public.h changes have been removed from this PR.

lrknox · 2021-03-22T16:08:54Z

@lrknox why the new commit f4a044b? Is it to undo a799020? I separated a799020 as @derobins requested, and can drop that commit when this is no longer WIP.

But I still don't see the need. When can #if H5_SIZEOF_INT64_T >= 8 be false? You realize all that code only does anything in such dead branches, right?

@seanm Sorry, did not see this comment. I was preparing to merge this when @derobins was satisfied, but is your WIP indicating you aren't ready for merge?

seanm · 2021-03-22T16:15:36Z

WIP indicating you aren't ready for merge?

Correct. For one, there are apparently tests failing, which I have yet to look at. There are also other C99 types I intend to clean up. I really created this PR to have a place to discuss which kinds of changes y'all would or would not accept.

Note, this is only assuming that `long double` exists, no assumptions about its size have been touched. Didn't remove any code that does things like test if `long double` and `double` have different sizes.

seanm · 2021-10-25T20:48:44Z

@qkoziol @derobins @lrknox @byrnHDF @epourmal OK, I found what was making tests fail. I think this is ready now.

lrknox · 2021-10-26T02:23:37Z

All checks passed after the clang-format change commit. See https://github.com/HDFGroup/hdf5/actions/runs/1382094984.

* Committing clang-format changes * Assume C99 fixed sized ints exist, use them * Assume H5_SIZEOF_LONG_DOUBLE != 0, `long double` has existed since C89 Note, this is only assuming that `long double` exists, no assumptions about its size have been touched. Didn't remove any code that does things like test if `long double` and `double` have different sizes. * Committing clang-format changes Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Sean McBride <sean@rogue-research.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

seanm requested review from derobins, fortnern, jhendersonHDF, jrmainzer, qkoziol, soumagne and vchoi-hdfgroup as code owners March 13, 2021 22:33

derobins requested changes Mar 14, 2021

View reviewed changes

qkoziol approved these changes Mar 15, 2021

View reviewed changes

seanm force-pushed the c99-fixed-sized-ints branch from 27a2a6f to 18d596a Compare March 15, 2021 19:38

seanm requested review from bmribler, brtnfld, byrnHDF and epourmal as code owners March 15, 2021 19:38

byrnHDF requested changes Mar 16, 2021

View reviewed changes

byrnHDF reviewed Mar 17, 2021

View reviewed changes

byrnHDF self-requested a review March 17, 2021 13:00

brtnfld approved these changes Mar 17, 2021

View reviewed changes

qkoziol approved these changes Mar 18, 2021

View reviewed changes

byrnHDF reviewed Mar 19, 2021

View reviewed changes

src/H5Ztrans.c Outdated Show resolved Hide resolved

byrnHDF reviewed Mar 19, 2021

View reviewed changes

src/H5Ztrans.c Outdated Show resolved Hide resolved

seanm force-pushed the c99-fixed-sized-ints branch from 53cfd9b to 13b12f2 Compare March 19, 2021 15:33

lrknox approved these changes Mar 22, 2021

View reviewed changes

byrnHDF mentioned this pull request Mar 29, 2021

Tools long double updates #522

Merged

seanm force-pushed the c99-fixed-sized-ints branch from f4a044b to ba227b0 Compare May 3, 2021 17:14

byrnHDF added the WIP Work in progress (please also convert PRs to draft PRs) label May 6, 2021

Committing clang-format changes

0b18451

seanm force-pushed the c99-fixed-sized-ints branch from c4d97ca to 7593be1 Compare August 12, 2021 16:57

seanm changed the title ~~WIP: Assume C99 fixed sized ints exist, use them~~ Assume C99 fixed sized ints exist, use them Aug 12, 2021

Merge branch 'develop' of https://github.com/seanm/hdf5 into develop

541039a

seanm force-pushed the c99-fixed-sized-ints branch from e56bc83 to 6845ec9 Compare August 24, 2021 19:56

seanm added 2 commits October 25, 2021 10:08

Merge remote-tracking branch 'upstream/develop' into develop

64a3631

Assume C99 fixed sized ints exist, use them

b206cab

seanm force-pushed the c99-fixed-sized-ints branch from b3d8d85 to a4d387f Compare October 25, 2021 16:21

Assume H5_SIZEOF_LONG_DOUBLE != 0, long double has existed since C89

8c1eea7

Note, this is only assuming that `long double` exists, no assumptions about its size have been touched. Didn't remove any code that does things like test if `long double` and `double` have different sizes.

seanm force-pushed the c99-fixed-sized-ints branch from 23df5ae to 8c1eea7 Compare October 25, 2021 16:56

Committing clang-format changes

73b52cf

byrnHDF approved these changes Oct 25, 2021

View reviewed changes

derobins approved these changes Nov 8, 2021

View reviewed changes

lrknox merged commit 9cea7c9 into HDFGroup:develop Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assume C99 fixed sized ints exist, use them #470

Assume C99 fixed sized ints exist, use them #470

seanm commented Mar 13, 2021

seanm commented Mar 13, 2021

qkoziol commented Mar 13, 2021

derobins left a comment

derobins commented Mar 14, 2021 •

edited

Loading

seanm commented Mar 14, 2021

qkoziol commented Mar 15, 2021

qkoziol left a comment

epourmal commented Mar 15, 2021 via email

seanm commented Mar 15, 2021

byrnHDF left a comment •

edited

Loading

seanm commented Mar 16, 2021

byrnHDF commented Mar 17, 2021

byrnHDF commented Mar 17, 2021

byrnHDF left a comment

brtnfld left a comment

seanm commented Mar 17, 2021

byrnHDF commented Mar 17, 2021

lrknox commented Mar 19, 2021

seanm commented Mar 22, 2021

lrknox commented Mar 22, 2021

lrknox commented Mar 22, 2021

seanm commented Mar 22, 2021

seanm commented Oct 25, 2021

lrknox commented Oct 26, 2021

Assume C99 fixed sized ints exist, use them #470

Assume C99 fixed sized ints exist, use them #470

Conversation

seanm commented Mar 13, 2021

seanm commented Mar 13, 2021

qkoziol commented Mar 13, 2021

derobins left a comment

Choose a reason for hiding this comment

derobins commented Mar 14, 2021 • edited Loading

seanm commented Mar 14, 2021

qkoziol commented Mar 15, 2021

qkoziol left a comment

Choose a reason for hiding this comment

epourmal commented Mar 15, 2021 via email

seanm commented Mar 15, 2021

byrnHDF left a comment • edited Loading

Choose a reason for hiding this comment

seanm commented Mar 16, 2021

byrnHDF commented Mar 17, 2021

byrnHDF commented Mar 17, 2021

byrnHDF left a comment

Choose a reason for hiding this comment

brtnfld left a comment

Choose a reason for hiding this comment

seanm commented Mar 17, 2021

byrnHDF commented Mar 17, 2021

lrknox commented Mar 19, 2021

seanm commented Mar 22, 2021

lrknox commented Mar 22, 2021

lrknox commented Mar 22, 2021

seanm commented Mar 22, 2021

seanm commented Oct 25, 2021

lrknox commented Oct 26, 2021

derobins commented Mar 14, 2021 •

edited

Loading

byrnHDF left a comment •

edited

Loading