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]

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);
}

 

[yayalee1983]

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

 

[Kai-Uwe Bux]

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

 

[Ian Collins]

(e-mail address removed) wrote:

Please don't top post.

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.

 

[Joel Yliluoma]

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]

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

 

[James Kanze]

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);

More than just "avoided". It's illegal, and will not compile.

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.

I don't see where he copies anything. If "image" doesn't
support copy and assignment, he needs a pointer. If the names
here mean what they seem to mean, it's almost certain that image
has identity, and thus, it is highly probable that it doesn't
support copy and assignment.

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);
}

If image supports it, fine.

If image doesn't, of course, his code (even correcting the name
new) doesn't work. He needs something like:

void
PrettyMenu::changebackgroud(
std::istream& newBackground )
{
image* tmp = new image( newBackground ) ;
lock( &myMutex ) ;
delete myBackground ;
myBackground = tmp ;
unlock( &myMutex ) ;
}

(I've changed the name of the pointer variable to something more
intelligent as well.)

Of course, a scoped lock of some sort would be preferable as
well (and necessary if the new is in the locked area).

 

[James Kanze]

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

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

 

[Kai-Uwe Bux]

More than just "avoided". It's illegal, and will not compile.


I don't see where he copies anything.

True, I was sloppy. What I was refering to is that the pointee is not shared
between several PrettyMenu objects: each object is the exclusive owner of
its image. In those cases, I just don't see any need for a pointer. Values
would do just fine. Of course, the PrettyMenu class might have semantics
that involves identity. However, I do not see why images should be like
that too. It seems to be perfectly reasonable that images should support
IO, copying and assignment.

If "image" doesn't
support copy and assignment, he needs a pointer.

No, he needs to change the image class so that is has copy constructor and
assignment operator

If the names
here mean what they seem to mean, it's almost certain that image
has identity, and thus, it is highly probable that it doesn't
support copy and assignment.

Where do you get that meaning of "image"? I could see that for "PrettyMenu",
but not for "image".


[snip]


Best

Kai-Uwe Bux