Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > Why is it -- or is it -- bad to cast like the following code?

Reply
Thread Tools

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

 
 
cindy.r.mcgee@gmail.com
Guest
Posts: n/a
 
      08-23-2006
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;
}

 
Reply With Quote
 
 
 
 
Markus Grueneis
Guest
Posts: n/a
 
      08-23-2006
http://www.velocityreviews.com/forums/(E-Mail Removed) schrieb:
> 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)
 
Reply With Quote
 
 
 
 
Victor Bazarov
Guest
Posts: n/a
 
      08-23-2006
(E-Mail Removed) wrote:
> 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
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask


 
Reply With Quote
 
Noah Roberts
Guest
Posts: n/a
 
      08-23-2006

(E-Mail Removed) wrote:
> 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.
> .
> :
> protected:
> LONG left;
> LONG top;
> LONG right;
> LONG bottom;
> }


 
Reply With Quote
 
Howard
Guest
Posts: n/a
 
      08-23-2006

<(E-Mail Removed)> wrote in message
news:(E-Mail Removed) ups.com...
> 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



 
Reply With Quote
 
Mark P
Guest
Posts: n/a
 
      08-23-2006
Howard wrote:
> <(E-Mail Removed)> wrote in message
> news:(E-Mail Removed) ups.com...
>> 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...
>


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.
 
Reply With Quote
 
Steve Pope
Guest
Posts: n/a
 
      08-24-2006
Noah Roberts <(E-Mail Removed)> wrote:

>(E-Mail Removed) wrote:


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


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
 
Reply With Quote
 
Clark S. Cox III
Guest
Posts: n/a
 
      08-24-2006
Noah Roberts wrote:
> (E-Mail Removed) wrote:
>> 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.


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

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


--
Clark S. Cox III
(E-Mail Removed)
 
Reply With Quote
 
David Harmon
Guest
Posts: n/a
 
      08-24-2006
On 23 Aug 2006 13:57:17 -0700 in comp.lang.c++,
(E-Mail 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.


 
Reply With Quote
 
Frederick Gotham
Guest
Posts: n/a
 
      08-24-2006
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).

--

Frederick Gotham
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Bad media, bad files or bad Nero? John Computer Information 23 01-08-2008 09:17 PM
findcontrol("PlaceHolderPrice") why why why why why why why why why why why Mr. SweatyFinger ASP .Net 2 12-02-2006 03:46 PM
ActiveX apologetic Larry Seltzer... "Sun paid for malicious ActiveX code, and Firefox is bad, bad bad baad. please use ActiveX, it's secure and nice!" (ok, the last part is irony on my part) fernando.cassia@gmail.com Java 0 04-16-2005 10:05 PM
24 Season 3 Bad Bad Bad (Spoiler) nospam@nospam.com DVD Video 12 02-23-2005 03:28 AM
24 Season 3 Bad Bad Bad (Spoiler) nospam@nospam.com DVD Video 0 02-19-2005 01:10 AM



Advertisments