Rosegarden

On 02/10/2013 10:21 AM, Tim Munro wrote:
> I've dealt with the issue by cobbling together an ugly, complicated copy
> constructor for the PluginContainer class.
>
> The resulting code appears to work, but I have no idea what sort of memory
> leaks or other future train wrecks I might have introduced while beating
> it into submission.

   It's a good idea to avoid the need for a custom copy ctor if at all 
possible.  Copy ctors are tricky to get right.  If I had time, I'd take 
a look at this situation and see if there are alternatives.  Instead, 
I'll just pontificate on copy ctors for a bit.  Ignore if you've heard 
it all before....

   The first alternative that I usually try for is to remove the need 
for a copy ctor altogether.  This can be achieved at least two ways: 
don't copy the object, or make everything in the object copy itself. 
Why is this plugin container being copied in the first place?  It sounds 
like that would be a less than desirable thing to do (potentially 
expensive in terms of CPU and memory).  Is there a way to change the 
design so that no copy is necessary?  You can find every place where a 
copy occurs by making your copy ctor private.  (Unless, of course, this 
is so botched up that the object is copying itself.  Yikes!  In that 
case, leaving it declared but undefined will cause the linker to 
complain.)  Once these copies have been identified, an attempt can be 
made to remove them.

   Making everything in the object copy itself can be achieved in many 
ways.  One way is to make everything an object instead of having 
pointers.  Another is to use smart pointers (like shared_ptr) which 
provide a reference counting mechanism.

   In the end, though, a copy ctor might indeed be the right thing to do 
(and yours might be exactly what is needed).  In that case make sure 
that the copy ctor copies *everything* in a way that makes sense.  It 
must create an object that is identical to the original in every way 
(read through the regular ctors and the member objects to create a 
checklist of things you need to do in your copy ctor).  Also, make sure 
you declare an operator=().  You can make it private to prevent anyone 
from using it.  You can also leave it undefined so you'll get a linker 
error in case the class itself uses it.  Having a copy ctor and no 
operator=() is almost always an error.  (Actually implementing an 
operator= is a bit tricky as it must deal properly with self-assignment.)

   For more, read up on the Rule of Three:

https://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

   (Which in C++11 is now the Rule of Five what with the new move ctor 
and move assignment.)

   Another great source is Lippman's C++ Primer 5th ed. (2013) Chapter 13.

   The crash symptoms you describe sound like a case of a class that has 
pointer members (directly or indirectly), does a delete in the dtor, but 
has no copy ctor or op= defined.  Then somebody does a copy (or 
assignment).  This is the perfect recipe for a crash.  The Rule of Three 
should never be taken lightly like this.

Ted.

Ted:

Good comments on copy ctors.

> Having a copy ctor and no
> operator=() is almost always an error.  (Actually implementing an
> operator= is a bit tricky as it must deal properly with self-assignment.)

We've had a few bugs because copy ctor and operator=() didn't agree,
usually because one was updated but the other wasn't.  I don't think we
have any situations where they shouldn't completely agree.

One way to make them always agree is to have operator=() use explicit dtor
and placement new.  Then any change to the copy ctor is automatically
shared.  This recipe only requires replacing Classname:

    Classname& operator=(const Classname& other)
    {
      if (this != &other) {
	this->~Classname();
	new(this) Classname(other);
      }
      return *this;
    }

Caveat: It's not meant for base classes.

	Tom Breton (Tehom)

Ted:

Ted Felix wrote:
> I'll just pontificate on copy ctors for a bit.  Ignore if you've heard
> it all before....

I haven't and I very much appreciate the feedback.

> The first alternative that I usually try for is to remove the need
> for a copy ctor altogether.

I will try to do this.  The implicit copy ctor wasn't up to the task, so
simply dropping in an explicit one seemed the path of least resistance.
However, since both Buss class and Instrument class inherit from
PluginContainer class, any mistakes there could generate all sorts of nasty
side effects.  I will see if I can find some other way around the problem.

> Also, make sure you declare an operator=().  You can make it private to
> prevent anyone from using it.  You can also leave it undefined so you'll
> get a linker error in case the class itself uses it.

For what it's worth, I did declare a private, undefined operator=(), and the
program compiled without a hitch.

Clearly the patch I submitted should not be used as is.  I look forward to
getting this thing resolved, so I can get back to actually writing some music.

Tim Munro

On 02/10/2013 06:53 PM, Tim Munro wrote:
>> Also, make sure you declare an operator=().  You can make it private to
>> prevent anyone from using it.  You can also leave it undefined so you'll
>> get a linker error in case the class itself uses it.
>
> For what it's worth, I did declare a private, undefined operator=(), and the
> program compiled without a hitch.

   That's a real good sign.  That means the code isn't too far gone.

> Clearly the patch I submitted should not be used as is.

   Really appreciate the effort, though.  We need the help.  You might 
be really close, depending on the situation.

> I look forward to
> getting this thing resolved, so I can get back to actually writing some music.

   I hear ya.  Seems to always be the case with rg.

Ted.

On 02/10/2013 07:37 PM, Ted Felix wrote:

>> Clearly the patch I submitted should not be used as is.
>
>     Really appreciate the effort, though.  We need the help.  You might
> be really close, depending on the situation.

Probably won't get anywhere, but I have a minute, and I'm poking around 
to see if I see anything.  Will report.
-- 
D. Michael McIntyre

D. Michael McIntyre wrote:
> On 02/10/2013 07:37 PM, Ted Felix wrote:
>
>>> >>  Clearly the patch I submitted should not be used as is.
>> >
>> >       Really appreciate the effort, though.  We need the help.  You might
>> >  be really close, depending on the situation.
> Probably won't get anywhere, but I have a minute, and I'm poking around
> to see if I see anything.  Will report.
> -- D. Michael McIntyre

I think I've got it this time.  I was almost there anyway.  I found a way to add
and remove submaster busses without resorting to copying.

The Studio class already contained an addBuss() function, and now it also
contains a removeBuss() function.

Tim Munro

On 02/11/2013 03:06 PM, Tim Munro wrote:

> and remove submaster busses without resorting to copying.
>
> The Studio class already contained an addBuss() function, and now it also
> contains a removeBuss() function.

That looks like it should be a safer way of removing the old bus.

Just looking at this and having only a very vague understanding of 
what's going on, I wonder if this latest version doesn't wipe out 
whatever was on the original set of busses when you've added or 
subtracted one.  Isn't it just wiping the slate clean and producing a 
new slate of whatever the desired size is?

If so, and if that gets the job done, I don't have a real problem with 
that, but I feel there should probably be some kind of warning.

Or maybe I'm reading it wrong.  I just woke up, depressingly enough. 
8:00 PM and I just woke up.  I guess I won't be doing anything with THIS 
day off.

OTOH I guess I needed sleep, huh?  Apparently so!

-- 
D. Michael McIntyre

> If so, and if that gets the job done, I don't have a real problem with
> that, but I feel there should probably be some kind of warning.

So in actual practice hands-on, I don't see the problem I vaguely 
expected to see based on glancing at and partially understanding the code.

I didn't test this extensively at all, but I don't see any concerns.  If 
you did test this extensively yourself, and you're satisfied with this 
version, I'll clean up some nitpicky things and commit just in time for 
the 13.02 release.
-- 
D. Michael McIntyre

> The Studio class already contained an addBuss() function, and now it also
> contains a removeBuss() function.

So after fooling around a bit more, I ended up committing it without 
changing anything after all.  Some of the picky stuff I was going to go 
massage, you were just copying the spirit and style of the surrounding code.

Seems OK to me, and nobody else has any more time than I do to review 
and really dig, so let's go with it and see what blows up in the future!

It's called QUALITY CONTROL, and it's what we EXCEL at here at Rosegarden.
-- 
D. Michael McIntyre

D. Michael McIntyre wrote:
> On 02/11/2013 03:06 PM, Tim Munro wrote:
>
>> >  and remove submaster busses without resorting to copying.
>> >
>> >  The Studio class already contained an addBuss() function, and now it also
>> >  contains a removeBuss() function.
> That looks like it should be a safer way of removing the old bus.
>
> Just looking at this and having only a very vague understanding of
> what's going on, I wonder if this latest version doesn't wipe out
> whatever was on the original set of busses when you've added or
> subtracted one.  Isn't it just wiping the slate clean and producing a
> new slate of whatever the desired size is?

No, my priority was to preserve the content of the original busses wherever
possible.  That was also the intent of the original code.

Expanding the number of submasters now involves padding m_busses (in Studio)
with empty busses until the desired size is reached.  Shrinking the number of
submasters involves removing existing busses from the end of m_busses until the
desired size is reached.  The original content of m_busses is left undisturbed
wherever possible.

The original code would take a snapshot of m_busses, blow away the contents of
m_busses, and then repack it with new busses.  These new busses would be either
empties or duplicates of the originals, as needed.  Where the original code got
into trouble was when it tried to duplicate existing busses containing plugins.

My first plan involved simply upgrading the copy ctor so that the code could
proceed as intended.  But Ted wisely pointed out that this approach was fraught
with danger, so chose a different method.

Tim Munro

D. Michael McIntyre wrote:
> Some of the picky stuff I was going to go massage, you were just copying
> the spirit and style of the surrounding code.

Yeah, that was my intent.  I figured that way at least I wouldn't be degrading
anything.

Tim Munro

On Tue, 12 Feb 2013, Tim Munro wrote:

> D. Michael McIntyre wrote:
> > On 02/11/2013 03:06 PM, Tim Munro wrote:
> >
> >> >  and remove submaster busses without resorting to copying.
> >> >
> >> >  The Studio class already contained an addBuss() function, and now it also
> >> >  contains a removeBuss() function.
> > That looks like it should be a safer way of removing the old bus.
> >
> > Just looking at this and having only a very vague understanding of
> > what's going on, I wonder if this latest version doesn't wipe out
> > whatever was on the original set of busses when you've added or
> > subtracted one.  Isn't it just wiping the slate clean and producing a
> > new slate of whatever the desired size is?
> 
> No, my priority was to preserve the content of the original busses wherever
> possible.  That was also the intent of the original code.

I wouldn't see it as a bug if expanding/shrinking the number of
submasters would clear the plugins before, provided there was a warning
before.

If it can be done while preserving the plugins and their settings then
it would be perfect.

> No, my priority was to preserve the content of the original busses wherever
> possible.  That was also the intent of the original code.

I get it now.  I like the thinking behind that approach.

I committed the patch because it seemed to work, but I feel much more 
relaxed about it now.

I'm especially grateful that you worked out all the QAction nonsense and 
got that to work too.

I'm noticing the trend that Ted, Tom and Tim are a tenacious trio of 
technological techniques.  Rosegarden owes its good fortune to the 
letter T and the number 3.

Well then, I'll see about releasing this in the next week or so.  Your 
timing was perfect.
-- 
D. Michael McIntyre

On 02/12/2013 07:50 AM, Holger Marzen wrote:

> If it can be done while preserving the plugins and their settings then
> it would be perfect.

Tim Munro is your new best friend then.  Try it yourself.  Seemed to 
work quite well to me.
-- 
D. Michael McIntyre

>
> Seems OK to me, and nobody else has any more time than I do to review
> and really dig, so let's go with it and see what blows up in the future!
>
> It's called QUALITY CONTROL, and it's what we EXCEL at here at Rosegarden.
>


> I'm noticing the trend that Ted, Tom and Tim are a tenacious trio of
> technological techniques.  Rosegarden owes its good fortune to the letter T
> and the number 3.
>

Can I submit these as quotes of the week on LWN? They're just too good not
to share!

On Tue, 12 Feb 2013, D. Michael McIntyre wrote:

> On 02/12/2013 07:50 AM, Holger Marzen wrote:
> 
> > If it can be done while preserving the plugins and their settings then
> > it would be perfect.
> 
> Tim Munro is your new best friend then.  Try it yourself.  Seemed to 
> work quite well to me.

Works fine. Good work!

Regards
Holger

On 02/12/2013 10:18 AM, Daren Beattie wrote:

> Can I submit these as quotes of the week on LWN? They're just too good
> not to share!

Knock yourself out.
-- 
D. Michael McIntyre