Modern C++ dtor declarations #1830

seanm · 2022-06-28T21:17:46Z

No description provided.

seanm · 2022-06-28T22:10:25Z

I'll need someone's help to actually implement the copy assignment operators correctly.

seanm · 2022-06-29T16:24:48Z

@byrnHDF would you know how to implement the copy assignment operators in the second commit? I figure they should probably call close() like the dtor does...

byrnHDF · 2022-06-29T18:51:32Z

@byrnHDF would you know how to implement the copy assignment operators in the second commit? I figure they should probably call close() like the dtor does...

I'm not up-to-date on the latest conventions for these types of functions.

seanm · 2022-06-30T15:48:54Z

@byrnHDF who's the HDF5 C++ expert that I can ping?

seanm · 2022-07-08T17:31:59Z

@gnuoyd @derobins do you know an HDF5 C++ expert that I can ping?

bmribler · 2022-07-08T20:11:19Z

Not calling myself an expert, but I'm reviewing this PR. Binh-Minh

…

________________________________ From: Sean McBride ***@***.***> Sent: Friday, July 8, 2022 1:32 PM To: HDFGroup/hdf5 ***@***.***> Cc: Binh-Minh Ribler ***@***.***>; Review requested ***@***.***> Subject: Re: [HDFGroup/hdf5] WIP: Modern C++ dtor declarations (PR #1830) @gnuoyd<https://github.com/gnuoyd> @derobins<https://github.com/derobins> do you know an HDF5 C++ expert that I can ping? — Reply to this email directly, view it on GitHub<#1830 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJN634G6FV36DAKRC4DHVRTVTBQZVANCNFSM52DRST6A>. You are receiving this because your review was requested.Message ID: ***@***.***>

derobins · 2022-07-17T05:55:08Z

@gnuoyd @derobins do you know an HDF5 C++ expert that I can ping?

I can help, but I'm a bit underwater for the immediate future. I'll jump in as soon as I free up.

qkoziol

Looks sensible, but are there tests that exercise the changes?

seanm · 2022-08-01T20:21:53Z

Looks sensible, but are there tests that exercise the changes?

I didn't add any new tests, if that's what you're asking. I imagine the dtors are well-exercised by existing tests. As for the copy assignment operators, I'm not sure.

bradenmcd · 2022-08-15T00:25:52Z

c++/src/H5AbstractDs.h

@@ -68,7 +68,7 @@ class H5_DLLCPP AbstractDs {
    virtual H5std_string fromClass() const = 0;

    // Destructor
-    virtual ~AbstractDs();
+    virtual ~AbstractDs() = default;


For a virtual destructor, prefer putting this in the .cpp file; i.e.,:

AbstractDs::AbstractDs() = default;

That way, you don't end up generating a definition in every translation unit that includes the header.

I have never seen a codebase do that... I can't find much discussion about it either, except for this, where the answers are not so conclusive to me...

I'd like to RTFM on this if you could point me somewhere...

c++/src/H5ArrayType.h

c++/src/H5Exception.h

seanm · 2023-07-27T21:15:42Z

@bmribler not sure why you just tagged this as "approved"... it's a WIP and in fact it appears to be conflicted now...

seanm · 2023-07-27T21:36:16Z

OK, I removed the commit that was WIP, leaving only the 1 commit that modernizes the dtors.

This is now ready for review & merge IMHO.

lrknox · 2023-07-28T04:17:52Z

The PR didn't run tests directly, but the commit earlier today passed all tests except the clang_format check which failed due to 2 extra blank lines, which were removed in the subsequent commit.

bmribler · 2023-07-28T16:00:26Z

@bmribler not sure why you just tagged this as "approved"... it's a WIP and in fact it appears to be conflicted now...

My mistake, @seanm

bmribler · 2023-07-28T16:07:06Z

I'll need someone's help to actually implement the copy assignment operators correctly.

@seanm, I'm sorry, I can do it after this modernizes the dtors PR is merged.

- Replaced a bunch of empty dtors with `= default` - Removed deprecated `throw()`. In C++11, dtors are `noexcept` by default.

seanm · 2023-09-08T20:14:00Z

@bmribler @byrnHDF friendly ping...

bmribler · 2023-09-12T21:11:28Z

@seanm you meant for the copy assignment operators?

seanm · 2023-09-13T00:04:36Z

@seanm you meant for the copy assignment operators?

No, those were moved to #3306

This PR is ready to merge IMHO.

* C++ dtor modernization - Replaced a bunch of empty dtors with `= default` - Removed deprecated `throw()`. In C++11, dtors are `noexcept` by default. *

* Update upload- artifact to match download version (#3929) * Reorg and update options for doc and cmake config (#3934) * Add binary build for linux S3 (#3936) * Clean up Doxygen for szip functions and constants (#3943) * Replace off_t with HDoff_t internally (#3944) off_t is a 32-bit signed value on Windows, so we should use HDoff_t (which is __int64 on Windows) internally instead. Also defines HDftell on Windows to be _ftelli64(). * Fix chid_t to hid_t (#3948) * Fortran API work. (#3941) * - Added Fortran APIs: H5FGET_INTENT_F, H5SSELECT_ITER_CREATE_F, H5SSEL_ITER_GET_SEQ_LIST_F, H5SSELECT_ITER_CLOSE_F, H5S_mp_H5SSELECT_ITER_RESET_F - Added Fortran Parameters: H5S_SEL_ITER_GET_SEQ_LIST_SORTED_F, H5S_SEL_ITER_SHARE_WITH_DATASPACE_F - Added tests for new APIs - Removed H5F C wrapper stubs - Documentation misc. cleanup. * Add the user test program in HDFFV-9174 for committed types. (#3937) Add the user test program for committed types in HDFFV-9174 * Remove cached datatype conversion path table entries on file close (#3942) * fixed BIND name (#3957) * update H5Ssel_iter_reset_f test * Change 'extensible' to 'fixed' in H5FA code (#3964) * RF: move codespell configuration into .codespellrc so could be used locally as well (#3958) * Add RELEASE.txt note for the fix for issue #1256 (#3955) * Fix doxygen errors (#3962) * Add API support for Fortran MPI_F08 module definitions. (#3959) * revert to using c-stub for _F08 MPI APIs * use mpi compiler wrappers for cmake and nvhpc * Added a GitHub Codespaces configuration. (#3966) * Fixed XL and gfortran errors (#3968) * h5 compiler wrappers now pass all arguments passed to it to the compile line (#3954) * The issue was that the "allargs" variable was not being used in the final command of the compiler wrapper. Any entries containing an escaped quote (\", \') or other non-matching argument (*) would not be passed to the compile line. I have fixed this problem by ensuring all arguments passed to the compiler wrapper are now included in the compile line. * Add binary testing to CI testing (#3971) * Replace 'T2' with ' ' to avoid failure to match expected output due to (#3975) * Clarify vlen string datatype message (#3950) * append '-WF,' when passing C preprocessor directives to the xlf compiler (#3976) * Create CITATION.cff (#3927) Add citation source based on http://web.archive.org/web/20230610185232/https://portal.hdfgroup.org/display/knowledge/How+do+I+properly+cite+HDF5%The space difference in the Fortran examples must be fixed to match the expected output for compression filter examples. * corrected warning: implicit conversion changes signedness (#3982) * Skip mac bintest until more reliable (#3983) * Make platform specific test presets for windows and macs (#3988) * chore: fix typo (#3989) * Add a missing left parenthesis in RELEASE.txt. (#3990) * Remove ADB signature from RELEASE.txt. (#3986) * Bump the github-actions group with 6 updates (#3981) * Sync API tests with vol-tests (#3940) * Fix for github issue #2414: segfault when copying dataset with attrib… (#3967) * Fix for github issue #2414: segfault when copying dataset with attributes. This also fixes github issue #3241: segfault when copying dataset. Need to set the location via H5T_set_loc() of the src datatype when copying dense attributes. Otherwise the vlen callbacks are not set up therefore causing seg fault when doing H5T_convert() -> H5T__conv_vlen(). * Fix broken links caused by examples relocation. (#3995) * Add abi-complience check and upload to releases (#3996) * Fix h5watch test failures to ignore system warnings on ppc64le. (#3997) * Remove oneapi/clang compiler printf() type warning. (#3994) * Updated information about obtaining the HDF5 source code to use the repos. (#3972) * Fix overwritten preset names (#4000) * Fix incompatible pointer type warnings in object reference examples (#3999) * Fix build issue and some warnings in H5_api_dataset_test.c (#3998) * Modern C++ dtor declarations (#1830) * C++ dtor modernization - Replaced a bunch of empty dtors with `= default` - Removed deprecated `throw()`. In C++11, dtors are `noexcept` by default. * remove incorrect check for environ (#4002) * Add a missing file into Makefile.am for MinGW Autotools build error. (#4004) * Issue #1824: Replaced most remaining sprintf with safer snprint (#4003) * Add hl and cpp ABI reports to daily build (#4006) * Don't add files and directories with names that begin with ., or that match *autom4te* to release tar & zip files. (#4009) * Fix some output issues with ph5diff (#4008) * Update install texts (#4010) * Add C in project line for CMake to fix #4012. (#4014) * separate out individual checks for string removal (#4015) * Add compound subset ops on attributes to API tests (#4005) ---------

seanm requested review from bmribler, byrnHDF, ChristopherHogan, gnuoyd and derobins as code owners June 28, 2022 21:17

byrnHDF approved these changes Jun 29, 2022

View reviewed changes

qkoziol reviewed Jul 31, 2022

View reviewed changes

bradenmcd reviewed Aug 15, 2022

View reviewed changes

c++/src/H5ArrayType.h Show resolved Hide resolved

bradenmcd reviewed Aug 15, 2022

View reviewed changes

c++/src/H5Exception.h Outdated Show resolved Hide resolved

seanm force-pushed the modern-dtor branch from 459075b to 7079439 Compare October 13, 2022 19:25

seanm requested review from lrknox, fortnern, jhendersonHDF, vchoi-hdfgroup and raylu-hdf as code owners October 13, 2022 19:25

derobins self-assigned this Feb 17, 2023

seanm force-pushed the modern-dtor branch from dc442e8 to e846c0e Compare March 13, 2023 01:41

seanm requested review from mattjala and brtnfld as code owners March 13, 2023 01:41

derobins marked this pull request as draft May 25, 2023 14:21

derobins added the WIP Work in progress (please also convert PRs to draft PRs) label May 25, 2023

seanm force-pushed the modern-dtor branch 3 times, most recently from 23b6895 to 543ed8e Compare June 16, 2023 13:38

bmribler approved these changes Jul 27, 2023

View reviewed changes

seanm force-pushed the modern-dtor branch from 7a3dff8 to 8be2ca8 Compare July 27, 2023 21:30

seanm marked this pull request as ready for review July 27, 2023 21:35

seanm requested a review from glennsong09 as a code owner July 27, 2023 21:35

seanm changed the title ~~WIP: Modern C++ dtor declarations~~ Modern C++ dtor declarations Jul 27, 2023

bmribler self-requested a review July 28, 2023 16:00

C++ dtor modernization

791d4c9

- Replaced a bunch of empty dtors with `= default` - Removed deprecated `throw()`. In C++11, dtors are `noexcept` by default.

seanm force-pushed the modern-dtor branch from aed2205 to 791d4c9 Compare August 2, 2023 23:12

Committing clang-format changes

cbac9c4

bmribler approved these changes Feb 9, 2024

View reviewed changes

lrknox merged commit 623907f into HDFGroup:develop Feb 9, 2024

lrknox pushed a commit to lrknox/hdf5 that referenced this pull request Feb 15, 2024

Modern C++ dtor declarations (HDFGroup#1830)

6331dda

* C++ dtor modernization - Replaced a bunch of empty dtors with `= default` - Removed deprecated `throw()`. In C++11, dtors are `noexcept` by default. *

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modern C++ dtor declarations #1830

Modern C++ dtor declarations #1830

seanm commented Jun 28, 2022

seanm commented Jun 28, 2022

seanm commented Jun 29, 2022

byrnHDF commented Jun 29, 2022

seanm commented Jun 30, 2022

seanm commented Jul 8, 2022

bmribler commented Jul 8, 2022 via email

derobins commented Jul 17, 2022

qkoziol left a comment

seanm commented Aug 1, 2022

bradenmcd Aug 15, 2022

seanm Oct 13, 2022

seanm commented Jul 27, 2023

seanm commented Jul 27, 2023

lrknox commented Jul 28, 2023

bmribler commented Jul 28, 2023

bmribler commented Jul 28, 2023 •

edited

Loading

seanm commented Sep 8, 2023

bmribler commented Sep 12, 2023

seanm commented Sep 13, 2023

Modern C++ dtor declarations #1830

Modern C++ dtor declarations #1830

Conversation

seanm commented Jun 28, 2022

seanm commented Jun 28, 2022

seanm commented Jun 29, 2022

byrnHDF commented Jun 29, 2022

seanm commented Jun 30, 2022

seanm commented Jul 8, 2022

bmribler commented Jul 8, 2022 via email

derobins commented Jul 17, 2022

qkoziol left a comment

Choose a reason for hiding this comment

seanm commented Aug 1, 2022

bradenmcd Aug 15, 2022

Choose a reason for hiding this comment

seanm Oct 13, 2022

Choose a reason for hiding this comment

seanm commented Jul 27, 2023

seanm commented Jul 27, 2023

lrknox commented Jul 28, 2023

bmribler commented Jul 28, 2023

bmribler commented Jul 28, 2023 • edited Loading

seanm commented Sep 8, 2023

bmribler commented Sep 12, 2023

seanm commented Sep 13, 2023

bmribler commented Jul 28, 2023 •

edited

Loading