hdf5 icon indicating copy to clipboard operation
hdf5 copied to clipboard

Published 20 hours ago • verified publisher icon HDFGroup

WIP: Modern C++ dtor declarations

Open seanm opened this issue 2 years ago • 8 comments

seanm avatar Jun 28 '22 21:06 seanm

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

seanm avatar Jun 28 '22 22:06 seanm

@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...

seanm avatar Jun 29 '22 16:06 seanm

@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.

byrnHDF avatar Jun 29 '22 18:06 byrnHDF

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

seanm avatar Jun 30 '22 15:06 seanm

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

seanm avatar Jul 08 '22 17:07 seanm

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)

@gnuoydhttps://github.com/gnuoyd @derobinshttps://github.com/derobins do you know an HDF5 C++ expert that I can ping?

— Reply to this email directly, view it on GitHubhttps://github.com/HDFGroup/hdf5/pull/1830#issuecomment-1179218793, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AJN634G6FV36DAKRC4DHVRTVTBQZVANCNFSM52DRST6A. You are receiving this because your review was requested.Message ID: @.***>

bmribler avatar Jul 08 '22 20:07 bmribler

@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.

derobins avatar Jul 17 '22 05:07 derobins

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.

seanm avatar Aug 01 '22 20:08 seanm

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

seanm avatar Jul 27 '23 21:07 seanm

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.

seanm avatar Jul 27 '23 21:07 seanm

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.

lrknox avatar Jul 28 '23 04:07 lrknox

@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 avatar Jul 28 '23 16:07 bmribler

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.

bmribler avatar Jul 28 '23 16:07 bmribler

@bmribler @byrnHDF friendly ping...

seanm avatar Sep 08 '23 20:09 seanm

@seanm you meant for the copy assignment operators?

bmribler avatar Sep 12 '23 21:09 bmribler

@seanm you meant for the copy assignment operators?

No, those were moved to https://github.com/HDFGroup/hdf5/pull/3306

This PR is ready to merge IMHO.

seanm avatar Sep 13 '23 00:09 seanm