Fix rule of three for SampleBuffer, SampleTCO #5727

Spekular · 2020-10-22T23:18:44Z

Custom destruction, copy constructors, and copy assignment should usually all be implemented for a class that implements any one of them. These changes are needed for #5524, but I decided to move them to a separate PR to split the review burden, especially since it's important (and tricky, IMO) to get these correct.

Status as of latest commit

Operator	SampleBuffer	SampleTCO
Copy Constructor	Added	Added
Copy Assignment	Added	Deleted

LmmsBot · 2020-10-22T23:24:49Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.70%2Bgfe4e118-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.70%2Bgfe4e1188b-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.70%2Bgfe4e1188b-mingw-win64.exe (build link)

macOS

macOS : lmms-1.3.0-alpha.1.70%2Bgfe4e1188b-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://11836-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.70%2Bgfe4e118-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11836?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://11835-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.70%2Bgfe4e1188b-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11835?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://11837-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.70%2Bgfe4e1188b-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11837?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://11839-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.70%2Bgfe4e1188b-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/11839?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "8789faf5632fab25b71199ed7c8fa954cb8918fd"}

DomClark · 2020-11-01T21:03:01Z

The rule of three (or five these days, now we have move constructors and assignment operators) doesn't say that objects have to be copyable or moveable; rather it says that if any of these functions are user-defined, the others should be user-defined too, because the compiler-generated ones are likely to be incorrect. If you write a destructor, you are probably trying to dispose of a resource when the object is destructed. The compiler-generated constructors and assignment operators will copy the resource handle, meaning it will be disposed of twice, and the compiler-generated assignment operators will overwrite the existing handle without disposing of it. If a particular constructor or assigment operator doesn't make sense, or you're not sure what to do at the moment, = delete is a perfectly acceptable definition. The goal is primarily to get rid of the compiler's own version.

SampleBuffer::operator= leaks the original data. You might like to look into the copy-and-swap idiom as a way to implement this.

…ethod

… read in copy constructor

Spekular · 2020-12-01T23:25:36Z

Will fix merge conflicts as soon as I know that there aren't bigger issues remaining!

include/SampleBuffer.h

include/SampleTrack.h

src/core/SampleBuffer.cpp

src/tracks/SampleTrack.cpp

src/core/SampleBuffer.cpp

include/SampleBuffer.h

src/core/SampleBuffer.cpp

Co-authored-by: Dominic Clark <[email protected]>

src/core/SampleBuffer.cpp

... by switching from an initializer list to manual assignments.

Spekular · 2020-12-06T21:44:04Z

@DomClark @IanCaio any remaining objections?

DomClark · 2020-12-06T22:44:53Z

src/core/SampleBuffer.cpp

+	memcpy(m_origData, orig.m_origData, m_origFrames * BYTES_PER_FRAME);
+	memcpy(m_data, orig.m_data, m_frames * BYTES_PER_FRAME);


orig.m_origData or orig.m_data can be null, such as for a default-constructed SampleBuffer. Calling memcpy with a null pointer is undefined behaviour, even if the size is zero.

In addition to the comment above, MOST implementations of memcpy does nothing if the length is 0, but it's not guaranteed that ALL implementations do nothing.

Hopefully fixed in latest commit

DomClark · 2020-12-06T22:48:35Z

src/tracks/SampleTrack.cpp

+SampleTCO::SampleTCO(const SampleTCO& orig) :
+	SampleTCO(orig.getTrack())
+{
+	// TODO: This creates a new SampleBuffer for the new TCO, eating up memory
+	// & eventually causing performance issues. Letting tracks share buffers
+	// when they're identical would fix this, but isn't possible right now.
+	m_sampleBuffer = new SampleBuffer(*orig.m_sampleBuffer);
+	m_isPlaying = orig.m_isPlaying;
+}


The constructor delegated to here initialises m_sampleBuffer with a newly allocated, default-constructed SampleBuffer. It is then leaked when this constructor overwrites it.

Hopefully fixed in latest commit

IanCaio · 2020-12-07T16:31:36Z

m_varLock is still not recursive. I'm having second thoughts whether it should though. If we call the copy assignment from a method that has locked the sample buffer we would deadlock, but we should only be locking the sample buffer when accessing it (which means we shouldn't be copy assigning to it until we are done and unlock). So I think we are good having it non-recursive.

I personally think it's more easily understandable to use the tryLock approach on the deadlock prevention: the address approach works just as well, but for a reader it might be confusing at first to know why it's done that way. But it's no big deal and it can be kept that way.

I'll approve it, just want to review it with more time later.

Spekular · 2020-12-07T17:04:01Z

m_varLock is still not recursive.

Wow, I totally forgot about that. As you say though, it doesn't seem necessary.

I personally think it's more easily understandable to use the tryLock approach on the deadlock prevention: the address approach works just as well, but for a reader it might be confusing at first to know why it's done that way.

Copying the std::swap() method would require:

While both not locked
- Try to lock first, then second. Release any acquired locks if we fail.
- Try to lock second, then first. Release any acquired locks if we fail.

With our bracket standards this would be a lot of lines.

But it's no big deal and it can be kept that way.

👍

src/core/SampleBuffer.cpp

Spekular · 2020-12-13T15:45:13Z

@DomClark and @IanCaio, are you happy with the fixes in the recent commits? If so, merge?

IanCaio

LGTM! 🚀

IanCaio · 2020-12-28T21:29:45Z

Any objections to merging this?

Veratil · 2020-12-28T21:32:51Z

Any objections to merging this?

I'm going to ignore style review this time and say no objections. 👍

DomClark

LGTM now. I made one comment, but it can be ignored if you want.

src/tracks/SampleTrack.cpp

Co-authored-by: Dominic Clark <[email protected]>

Spekular · 2020-12-29T22:01:14Z

Master CI fails in the same way as this, so I'll merge now 🎉

* Automatic formatting changes * Add copy constructor and assignemnt to SampleBuffer * Add copy constructor to SampleTCO * Delete SampleTCO copy assignment, initial work on SampleBuffer swap method * SampleBuffer: Finish(?) swap and use it for copy assignment, lock for read in copy constructor * Don't forget to unlock in copy assignment! * Formatting changes * Lock ordering in swap * Fix leak and constness Co-authored-by: Dominic Clark <[email protected]> * Fix multiplication style, ensure lock is held when necessary ... by switching from an initializer list to manual assignments. * Fixes from review * Avoid more undefined behavior * Update src/tracks/SampleTrack.cpp Co-authored-by: Dominic Clark <[email protected]>

Spekular added 3 commits October 23, 2020 00:36

Automatic formatting changes

f5cf414

Add copy constructor and assignemnt to SampleBuffer

420ba15

Add copy constructor to SampleTCO

34c98f1

Spekular added the needs code review A functional code review is currently required for this PR label Oct 23, 2020

Spekular mentioned this pull request Nov 17, 2020

Fixes style on "SampleBuffer.cpp" and "SampleBuffer.h" #5791

Merged

Spekular added 3 commits November 19, 2020 19:50

Delete SampleTCO copy assignment, initial work on SampleBuffer swap m…

8111fd1

…ethod

SampleBuffer: Finish(?) swap and use it for copy assignment, lock for…

46570e1

… read in copy constructor

Don't forget to unlock in copy assignment!

382102f

Spekular requested review from DomClark and IanCaio December 1, 2020 23:24

IanCaio requested changes Dec 2, 2020

View reviewed changes

DomClark reviewed Dec 2, 2020

View reviewed changes

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved

include/SampleBuffer.h Outdated Show resolved Hide resolved

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved

Spekular and others added 4 commits December 5, 2020 00:01

Formatting changes

7aeae94

Lock ordering in swap

ad574a4

Fix leak and constness

5d2e18e

Co-authored-by: Dominic Clark <[email protected]>

Merge branch 'master' into SBROT

6635bf7

Spekular changed the title ~~Fix rule of three for SampleBuffer, partly for SampleTCO~~ Fix rule of three for SampleBuffer, SampleTCO Dec 4, 2020

M374LX suggested changes Dec 5, 2020

View reviewed changes

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved

Spekular commented Dec 6, 2020

View reviewed changes

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved

Fix multiplication style, ensure lock is held when necessary

33d38b2

... by switching from an initializer list to manual assignments.

DomClark reviewed Dec 6, 2020

View reviewed changes

Fixes from review

e23da37

Merge branch 'master' into SBROT

c44d521

IanCaio reviewed Dec 10, 2020

View reviewed changes

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved

Avoid more undefined behavior

a9869ee

IanCaio approved these changes Dec 13, 2020

View reviewed changes

PhysSong approved these changes Dec 19, 2020

View reviewed changes

PhysSong requested a review from DomClark December 22, 2020 01:46

DomClark approved these changes Dec 29, 2020

View reviewed changes

src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved

Update src/tracks/SampleTrack.cpp

8789faf

Co-authored-by: Dominic Clark <[email protected]>

Spekular merged commit bf7c87d into LMMS:master Dec 29, 2020

Spekular deleted the SBROT branch December 29, 2020 22:01

PhysSong mentioned this pull request Dec 30, 2020

Knife Tool for Sample Clips (Again) #5524

Merged

1 task

		memcpy(m_origData, orig.m_origData, m_origFrames * BYTES_PER_FRAME);
		memcpy(m_data, orig.m_data, m_frames * BYTES_PER_FRAME);

Uh oh!

Fix rule of three for SampleBuffer, SampleTCO #5727

Fix rule of three for SampleBuffer, SampleTCO #5727

Uh oh!

Conversation

Spekular commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LmmsBot commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linux

Windows

macOS

Uh oh!

DomClark commented Nov 1, 2020

Uh oh!

Spekular commented Dec 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Spekular commented Dec 6, 2020

Uh oh!

DomClark Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhysSong Dec 6, 2020

Choose a reason for hiding this comment

Uh oh!

Spekular Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

DomClark Dec 6, 2020

Choose a reason for hiding this comment

Uh oh!

Spekular Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

IanCaio commented Dec 7, 2020

Uh oh!

Spekular commented Dec 7, 2020

Uh oh!

Uh oh!

Spekular commented Dec 13, 2020

Uh oh!

IanCaio left a comment

Choose a reason for hiding this comment

Uh oh!

IanCaio commented Dec 28, 2020

Uh oh!

Veratil commented Dec 28, 2020

Uh oh!

DomClark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Spekular commented Dec 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Spekular commented Oct 22, 2020 •

edited

Loading

LmmsBot commented Oct 22, 2020 •

edited

Loading

DomClark Dec 6, 2020 •

edited

Loading