Find error

[yayalee1983]

is there any error in the following code?

class PrettyMenu
{
public:
void changebackground(std::istream& new);
private:
mutex fmutex;
image *fimage;
int changenum;//record the change times
}

void changebackgroud(std::istream& new)
{
lock(&fmutex);
delete fimage;
++changnum;
fimage=new image(new);
unlock(&fmutex);
}

[Ian Collins]

ya*********@gmail.com wrote:

is there any error in the following code?

Several.

class PrettyMenu
{
public:
void changebackground(std::istream& new);
private:
mutex fmutex;
image *fimage;
int changenum;//record the change times
}

void changebackgroud(std::istream& new)
{
lock(&fmutex);
delete fimage;
++changnum;
fimage=new image(new);
unlock(&fmutex);
}

--
Ian Collins.

[yayalee1983]

It is just a paper test,not real code.and where?

Several.

[Ian Collins]

ya*********@gmail.com wrote:

Please don't top post.

>

>Several.

It is just a paper test,not real code.and where?

Homework?

What does your compiler tell you?

Hint: spot the inappropriate use of a keyword.

--
Ian Collins.

[Kai-Uwe Bux]

ya*********@gmail.com wrote:

is there any error in the following code?

class PrettyMenu
{
public:
void changebackground(std::istream& new);
private:
mutex fmutex;
image *fimage;
int changenum;//record the change times
}

void changebackgroud(std::istream& new)
{
lock(&fmutex);
delete fimage;
++changnum;
fimage=new image(new);
unlock(&fmutex);
}

I am not going to open the standard for this one: even if the code should
turn out formally correct, it should be rewritten anyway.

a) The use of "new" as the name for a parameter should be avoided,
especially if it leads to lines like

fimage = new image (new);

b) The member fimage is a pointer for no reason. Apparently, you allocate
and copy-construct the image anyway (at least in the method shown, the
objects behaves as though it is the exclusive owner of the image), so the
pointer buys you nothing but trouble.

c) The comment for the int is misleading: it makes you think that the int
stores a time stamp.
What about:

class PrettyMenu
{
public:
void changebackground(std::istream& istr);
private:
mutex fmutex;
image fimage;
int changenum;// count the changes
}

void changebackgroud(std::istream& istr)
{
lock(&fmutex);
istr >fimage;
++changnum;
unlock(&fmutex);
}
Best

Kai-Uwe Bux

[Joel Yliluoma]

On Thu, 29 Nov 2007 23:42:52 -0800 (PST), ya*********@gmail.com wrote:

is there any error in the following code?

Apart from the obvious problems preventing successful
compilation, I'll point one less obvious one:

void changebackgroud(std::istream& new)
{
lock(&fmutex);
delete fimage;
++changnum;
fimage=new image(new);
unlock(&fmutex);
}

If 'new' throws an exception here, fmutex remains
locked (unlock is not on the return part) and nobody
can lock it again.

Instead of littering all code with try...catch constructs,
I usually solve these by putting all the cleanup operations
in an ad-hoc scoped destructor.
For example:

void foo()
{
struct autolockmutex
{
mutex& m;
autolockmutex(mutex& mm) : m(mm) { lock(&m); }
~autolockmutex() { unlock(&m); }
} autounlock(fmutex);

delete...
...other stuff...
...a call to new...
...possibly some other stuff...
...but no mutex handling stuff needed anymore...
}
This way, no matter which way the function is exited*,
autolockmutex() will unlock the mutex at the end
of the function's scope.
Of course, this is so a common usecase that it is better
to create a global scoped lock class for the purpose.

To limit the scope in which the mutex is locked, you
can instantiate the autoclass inside a {} scope; it
will unlock it at the end of that scope, regardless
of the exit path.

*) except abort(), crash, etc...

--
Joel Yliluoma - http://iki.fi/bisqwit/

[Joel Yliluoma]

On Thu, 29 Nov 2007 23:42:52 -0800 (PST), ya*********@gmail.com wrote:

is there any error in the following code?

Apart from the obvious problems preventing successful
compilation, I'll point one less obvious one:

void changebackgroud(std::istream& new)
{
lock(&fmutex);
delete fimage;
++changnum;
fimage=new image(new);
unlock(&fmutex);
}

If 'new' throws an exception here, fmutex remains
locked (unlock is not on the return path) and nobody
can lock it again.

Instead of littering all code with try...catch constructs,
I usually solve these by putting all the cleanup operations
in an ad-hoc scoped destructor.
For example:

void foo()
{
struct autolockmutex
{
mutex& m;
autolockmutex(mutex& mm) : m(mm) { lock(&m); }
~autolockmutex() { unlock(&m); }
} autounlock(fmutex);

...all the normal function code here...
...except the mutex handling. ...
}
This way, no matter which way the function is exited*,
autolockmutex() will unlock the mutex at the end
of the function's scope.
Of course, this is so a common usecase that it is better
to create a global scoped lock class for the purpose.

To limit the scope in which the mutex is locked, you
can instantiate the autoclass inside a {} scope; it
will unlock it at the end of that scope, regardless
of the exit path.

*) except abort(), crash, etc...

--
Joel Yliluoma - http://iki.fi/bisqwit/

[James Kanze]

On Nov 30, 11:09 am, Joel Yliluoma <[e-mail]:

On Thu, 29 Nov 2007 23:42:52 -0800 (PST), [e-mail] wrote:

is there any error in the following code?

Apart from the obvious problems preventing successful
compilation, I'll point one less obvious one:

void changebackgroud(std::istream& new)
{
lock(&fmutex);
delete fimage;
++changnum;
fimage=new image(new);
unlock(&fmutex);
}

If 'new' throws an exception here, fmutex remains
locked (unlock is not on the return path) and nobody
can lock it again.

If 'new' throws an exception here, he can no longer access the
object, not even to destruct it. Loosing the lock is part of
the problem, but leaving an invalid pointer in fimage is also a
fatal error.

In short, he must do everything that can fail before the delete
(which leaves the pointer invalid). And the lock isn't
necessary except around the parts which can't fail, either.
(It's probably not desirable for the creation of the new image
to be protected by the lock either, since it's likely to be a
very long process.)

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]