Problem with cChannel's copy constructor

List overview All Threads
Download

newer

older

vdr slowness when browsing channels

[PATCH] vdr-1.2.6 off by ones in...

Marcel Wiesweg

4 Aug 2005 4 Aug '05

1:17 p.m.

Hi,

I just ran into a problem with cChannel's copy constructor. The problem is that this is actually not a copy constructor, because it sets the values to 0 instead of copying, and - most important - it behaves different from operator=.

It seems these two are not used in VDR except for one place (where I found this), cSectionHandler::SetChannel. The value stored by cSectionHandle has nid,tid,sid set to 0. In the line shp->channel = Channel ? *Channel : cChannel(); there seems only the operator= involved, but actually the copy constructor is invoked before the *Channel is given to opertator=. Attached is a short test program which illustrates the problem.

Solution is to make the copy constructor do copying.

Marcel

Attachments:

test.c (text/x-csrc — 670 bytes)

Show replies by date

Martin Cap

4 Aug 4 Aug

4:45 p.m.

Marcel Wiesweg wrote:

...

.... It seems these two are not used in VDR except for one place (where I found this), cSectionHandler::SetChannel. The value stored by cSectionHandle has nid,tid,sid set to 0. In the line shp->channel = Channel ? *Channel : cChannel(); there seems only the operator= involved, but actually the copy constructor is invoked before the *Channel is given to opertator=. Attached is a short test program which illustrates the problem.

Solution is to make the copy constructor do copying.

Hi,

first thing, in your code, you should return *this in the operator= (and check if &a equals 'this'), which, I assume, you just forgot.

Now what the compiler does, is the following : Assume this snippet from your code : ... 1: A *pointer= &object; 2: A second; ... 3: second = pointer ? *pointer : A();

@3: The compiler creates a anonymous, automatic object for class A on compile-time and copies members from *pointer via its copy-constructor into it (setting d=0!). Then, the operator= gets called for second with the temporary object as an argument (hence, copying d=0 into second).

Try the attached code, which'll make it clearer, I presume...

Apparently, the copy-constructor in cChannel needs explanation...

-- --- Regards, Martin Cap #include <stdio.h> #include <iostream> using std::cerr; using std::endl; class A { public: A() { cerr << __PRETTY_FUNCTION__ << " of " << this << endl; d=0; } //Define a copy constructor that does not copy A(const A &a) { if(this == &a) return; cerr << __PRETTY_FUNCTION__ << " of " << this << " with " << &a << endl; //cChannel's constructor does this: d=0; //Should be: //d=a.d; } A& operator=(const A &a) { cerr << __PRETTY_FUNCTION__ << " of " << this << " with " << &a << endl; if(this == &a) return *this; d=a.d; return *this; } int d; }; int main() { A object; object.d=5; A *pointer= &object; A second; cerr << "A object is " << &object << ", A *pointer is " << &*pointer << ", A second is " << &second << endl;; //Now trying to assign object to second. Should result in copying. //this assignment is done in sections.c:143 second = ((&*pointer) ? *pointer : A()); printf("Value of second's d is: %d\n\n", second.d); //no difference without the redirection BTW second = ((&*pointer) ? object : A()); //Value of d is 0, should be 5 printf("Value of second's d is: %d\n", second.d); return 0; }

Marcel Wiesweg

7:40 p.m.

...

Hi,

first thing, in your code, you should return *this in the operator= (and check if &a equals 'this'), which, I assume, you just forgot.

Yes, this code is just supposed to demonstrate the problem.

...

3: second = pointer ? *pointer : A();

@3: The compiler creates a anonymous, automatic object for class A on compile-time and copies members from *pointer via its copy-constructor into it (setting d=0!). Then, the operator= gets called for second with the temporary object as an argument (hence, copying d=0 into second).

Yes, running gdb made clear that something like this is happening, and I am willing to accept that this is according to the spec ;-)

...

Try the attached code, which'll make it clearer, I presume...

Apparently, the copy-constructor in cChannel needs explanation...

In sections.c:143, the code intends to copy the object, but the object is not copied, resulting in a bogus channel returned by cFilter::Channel(), and this is a bug in VDR.

I think that because of the possible implicit invocation of a copy constructor as seen in the example and in real code, a copy constructor should really be a copy constructor and equivalent to operator=. Otherwise you will run into bugs like above.

(See http://braincore.blogspot.com/2005/06/koding-big-four.html http://grammarian.homelinux.net/%7Empyne/weblog/tutorial/c%2B%2B-tidbits.htm... for two KDE hackers' comments on providing copy constructors.)

Marcel

Klaus Schmidinger

6 Aug 6 Aug

12:12 p.m.

Marcel Wiesweg wrote:

...

Hi,

I just ran into a problem with cChannel's copy constructor. The problem is that this is actually not a copy constructor, because it sets the values to 0 instead of copying, and - most important - it behaves different from operator=.

It seems these two are not used in VDR except for one place (where I found this), cSectionHandler::SetChannel. The value stored by cSectionHandle has nid,tid,sid set to 0. In the line shp->channel = Channel ? *Channel : cChannel(); there seems only the operator= involved, but actually the copy constructor is invoked before the *Channel is given to opertator=. Attached is a short test program which illustrates the problem.

Solution is to make the copy constructor do copying.

The "copy constructor" is actually used intentionally in cChannels::NewChannel(), where its sole purpose is to copy the actual transponder data. But you're of course right, a copy constructor should behave like a real copy constructor, and not serve a very special purpose.

I have therefore introduced cChannel::CopyTransponderData() for that purpose, and changed the copy ctor so that it calls operator=. See the attached patch and let me know if this works for you.

Klaus

--- channels.h 2005/05/28 13:57:08 1.32 +++ channels.h 2005/08/06 11:23:32 @@ -181,6 +181,7 @@ bool IsTerr(void) const { return cSource::IsTerr(source); } tChannelID GetChannelID(void) const { return tChannelID(source, nid, (nid || tid) ? tid : Transponder(), sid, rid); } int Modification(int Mask = CHANNELMOD_ALL); + void CopyTransponderData(const cChannel *Channel); bool SetSatTransponderData(int Source, int Frequency, char Polarization, int Srate, int CoderateH); bool SetCableTransponderData(int Source, int Frequency, int Modulation, int Srate, int CoderateH); bool SetTerrTransponderData(int Source, int Frequency, int Bandwidth, int Modulation, int Hierarchy, int CodeRateH, int CodeRateL, int Guard, int Transmission); --- channels.c 2005/05/29 10:32:38 1.42 +++ channels.c 2005/08/06 12:06:37 @@ -179,27 +179,13 @@

cChannel::cChannel(const cChannel &Channel) { - name = strdup(""); - shortName = strdup(""); - provider = strdup(""); - portalName = strdup(""); - *this = Channel; - vpid = 0; - ppid = 0; - apids[0] = 0; - dpids[0] = 0; - spids[0] = 0; - tpid = 0; - caids[0] = 0; - nid = 0; - tid = 0; - sid = 0; - rid = 0; - number = 0; - groupSep = false; - modification = CHANNELMOD_NONE; + name = NULL; + shortName = NULL; + provider = NULL; + portalName = NULL; linkChannels = NULL; refChannel = NULL; + *this = Channel; }

cChannel::~cChannel() @@ -265,6 +251,24 @@ return Result; }

+void cChannel::CopyTransponderData(const cChannel *Channel) +{ + if (Channel) { + frequency = Channel->frequency; + source = Channel->source; + srate = Channel->srate; + polarization = Channel->polarization; + inversion = Channel->inversion; + bandwidth = Channel->bandwidth; + coderateH = Channel->coderateH; + coderateL = Channel->coderateL; + modulation = Channel->modulation; + transmission = Channel->transmission; + guard = Channel->guard; + hierarchy = Channel->hierarchy; + } +} + bool cChannel::SetSatTransponderData(int Source, int Frequency, char Polarization, int Srate, int CoderateH) { // Workarounds for broadcaster stupidity: @@ -977,7 +981,8 @@ { if (Transponder) { dsyslog("creating new channel '%s,%s;%s' on %s transponder %d with id %d-%d-%d-%d", Name, ShortName, Provider, *cSource::ToString(Transponder->Source()), Transponder->Transponder(), Nid, Tid, Sid, Rid); - cChannel *NewChannel = new cChannel(*Transponder); + cChannel *NewChannel = new cChannel; + NewChannel->CopyTransponderData(Transponder); NewChannel->SetId(Nid, Tid, Sid, Rid); NewChannel->SetName(Name, ShortName, Provider); Add(NewChannel);

Marcel Wiesweg

6:08 p.m.

...

The "copy constructor" is actually used intentionally in cChannels::NewChannel(), where its sole purpose is to copy the actual transponder data. But you're of course right, a copy constructor should behave like a real copy constructor, and not serve a very special purpose.

I have therefore introduced cChannel::CopyTransponderData() for that purpose, and changed the copy ctor so that it calls operator=. See the attached patch and let me know if this works for you.

Yes, thanks, this works.

Marcel

...

Klaus

7083

Age (days ago)

7085

Last active (days ago)

vdr@linuxtv.org

4 comments

3 participants

tags (0)

participants (3)

Klaus Schmidinger
Marcel Wiesweg
Martin Cap