Why is it -- or is it -- bad to cast like the following code?

[cindy.r.mcgee]

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

Here's a brief excerpt with names changed to protect the innocent, er,
the IP:

class MyPoint
{
..
:
protected:
int x;
int y;
}

class MyRect
{
..
:
MyPoint & MyRect::TopLeft()
{
return *( ( MyPoint * ) this );
}

MyPoint & MyRect::BottomRight()
{
return *( ( MyPoint * ) this+1 );
}
..
:
protected:
LONG left;
LONG top;
LONG right;
LONG bottom;
}

 

[Markus Grueneis]

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

Here's a brief excerpt with names changed to protect the innocent, er,
the IP:

class MyPoint
{
.
:
protected:
int x;
int y;
}

class MyRect
{
.
:
MyPoint & MyRect::TopLeft()
{
return *( ( MyPoint * ) this );
}

MyPoint & MyRect::BottomRight()
{
return *( ( MyPoint * ) this+1 );
}
.
:
protected:
LONG left;
LONG top;
LONG right;
LONG bottom;
}

Curious Optimization???

(Note: this is different from Curious Templates)

 

[Victor Bazarov]

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

Here's a brief excerpt with names changed to protect the innocent, er,
the IP:

class MyPoint
{
.
protected:
int x;
int y;
}

;

class MyRect
{
.
MyPoint & MyRect::TopLeft()
{
return *( ( MyPoint * ) this );
}

MyPoint & MyRect::BottomRight()
{
return *( ( MyPoint * ) this+1 );
}
.
protected:
LONG left;

'LONG' is undefined.

LONG top;
LONG right;
LONG bottom;
}

;

Bad? I don't know. This code has undefined behaviour as far as C++
is concerned. Whether it's bad or not is for you to decide...

V

 

[Noah Roberts]

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

Here's a brief excerpt with names changed to protect the innocent, er,
the IP:

class MyPoint
{
.
:
protected:
int x;
int y;
}

class MyRect
{
.
:
MyPoint & MyRect::TopLeft()
{
return *( ( MyPoint * ) this );
}

MyPoint & MyRect::BottomRight()
{
return *( ( MyPoint * ) this+1 );
}

These are reinterpret casts. Dereferencing such a casted pointer is,
I'm pretty sure, totally undefined. Not only that but the reinterpret
cast of this+1 is almost certainly not what is wanted. They probably
thought ((MyPoint*)this) + 1 but this is even less predictable. This
is gross in so many ways.

In short, the behavior of this class is totally unpredictable. The
fact that the standard doesn't define a LONG is quite beside the
point...you don't need a definition to see the problems here.

MyRect should contain points that can be returned by these functions.
The code you are reviewing is unnecissarily terse and its nature is
undefined even if we can guess at the goal to some degree.

 

[Howard]

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

If you've forgotten why, then what made you decide it was bad? Sounds more
like a homework assignment to me...

-H

 

[Mark P]

If you've forgotten why, then what made you decide it was bad? Sounds more
like a homework assignment to me...

Her question seems sincere to me and hardly looks like homework. I
don't think students are likely to recast their questions in the context
of code reviews, nor are they likely to mention IP as part of their back
story.

 

[Steve Pope]

(e-mail address removed) wrote:

These are reinterpret casts. Dereferencing such a casted pointer is,
I'm pretty sure, totally undefined. Not only that but the reinterpret
cast of this+1 is almost certainly not what is wanted.

I don't see a cast of (this + 1) in the above code.

They probably thought ((MyPoint*)this) + 1

That's what they wrote.

Steve

 

[Clark S. Cox III]

These are reinterpret casts. Dereferencing such a casted pointer is,
I'm pretty sure, totally undefined. Not only that but the reinterpret
cast of this+1 is almost certainly not what is wanted. They probably
thought ((MyPoint*)this) + 1 but this is even less predictable. This
is gross in so many ways.

Just a nit to pick:
(MyPoint*)this + 1

is the same as:
((MyPoint*)this) + 1

 

[David Harmon]

On 23 Aug 2006 13:57:17 -0700 in comp.lang.c++,
(e-mail address removed) wrote,

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

The reason it's bad is that it throws away most of the potential
help and protection that C++ can give you with regard to accessing
the members of your class. By using a reinterpret cast (in the
form of a C cast) you are promising the compiler that you know what
you are doing and everything will be OK. If you make the slightest
slip, then everything will not be OK. If you write type-safe C++
without casts, the compiler gives you some assurances that things
will be OK instead of the other way around.

For example, you have class MyPoint with an x member coming before
the y member. Later you have a class MyRect which ought to have two
MyPoint members, corresponding to upper left and lower right.
Instead MyRect has four LONG members that by PURE COINCIDENCE might
possibly mimic the layout of two MyPoints. If you're lucky. If
"LONG" happens to be the same size as "int" and so forth. If nobody
decides to change one of them and forgets to change the other.

Nobody looking at class MyRect by itself has the FAINTEST HINT that
the order of those members, or anything else, depends on MyPoint.
Nobody looking at MyPoint can tell that MyRect depends on it. The
dependency is hidden in the cracks between them.

 

[Frederick Gotham]

posted:

I'm doing a code review and noticed some code that's not well-written,
and I've forgotten the reason why.

We don't have enough context -- I haven't got a clue what the code is
doing.

Here's a legitimate example though:

struct Coords {
int x, y;
};

struct Location {
int x,y;
char place_name[128];

Coords &GetCoords()
{
return (Coords&)*this;
}
};

int main()
{
Location london;

london.GetCoords().x = 5;
}

The first member of a POD is guaranteed to have the same address as the POD
object itself. POD structs which have a common initial sequence can be
accessed... blah blah blah (can't remember the exact quote).

 

[Victor Bazarov]

posted:

I'm doing a code review and noticed some code that's not
well-written, and I've forgotten the reason why.

We don't have enough context -- I haven't got a clue what the code is
doing.

Here's a legitimate example though:

struct Coords {
int x, y;
};

struct Location {
int x,y;
char place_name[128];

Coords &GetCoords()
{
return (Coords&)*this;
}
};

int main()
{
Location london;

london.GetCoords().x = 5;
}

The first member of a POD is guaranteed to have the same address as
the POD object itself. POD structs which have a common initial
sequence can be accessed... blah blah blah (can't remember the exact
quote).

Legitimate? Maybe. Sensible? I don't think so. Why would this be
any better than, say,

struct Coords {
int x, y;
};

struct Location {
Coords c;
...

?

V

 

[Frederick Gotham]

Victor Bazarov posted:

Legitimate? Maybe. Sensible? I don't think so. Why would this be
any better than, say,

struct Coords {
int x, y;
};

struct Location {
Coords c;
...

Of course, that would be better. However, I wonder if there's a funky reason
why the programmer chose not to do that (other than lack of intelligence).

 

[cindy.r.mcgee]

If you've forgotten why, then what made you decide it was bad? Sounds more
like a homework assignment to me...

Nope! My last homework assignment was back in 1994. I've been off in
C for a long time, then returned to C++ via MFC. I'm a bit rusty.

I appreciate everyone's comments; it's been a rough week.

Cindy

 

[cindy.r.mcgee]

I'll ask the programmer tomorrow when we have our review meeting. I
really wasn't clear on why, either.

Cindy

 

[cindy.r.mcgee]

Thank you for the detailed reply!

I didn't just want to say "it's bad code". The maintainability thing
was obvious, but the point it throws away most of the benefits of C++
was not (to me, yesterday, anyway).

Cindy

 

[red floyd]

[top posted reply redated]

Hi Cindy,

Please don't top-post in your replies.

See http://www.parashift.com/c++-faq-lite/how-to-post.html#faq-5.4

Thank you.

 

[Frederick Gotham]

Cindy posted:

I've been off in
C for a long time, then returned to C++ via MFC.

What a disgusting conduit through which to return to C++.

Word of advice: Do the exact opposite of everything that Microsoft does --
they have VERY poor programming practises.

 

[Howard]

Cindy posted:



What a disgusting conduit through which to return to C++.

Word of advice: Do the exact opposite of everything that Microsoft does --
they have VERY poor programming practises.

Geez... how about keeping your hatred of Microsoft out of this?
-H