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

Discussion in 'C++' started by cindy.r.mcgee@gmail.com, Aug 23, 2006.

  1. Guest

    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;
    }
     
    , Aug 23, 2006
    #1
    1. Advertising

  2. 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)
     
    Markus Grueneis, Aug 23, 2006
    #2
    1. Advertising

  3. 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
     
    Victor Bazarov, Aug 23, 2006
    #3
  4. Noah Roberts Guest

    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;
    > }
     
    Noah Roberts, Aug 23, 2006
    #4
  5. Howard Guest

    <> wrote in message
    news:...
    > 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
     
    Howard, Aug 23, 2006
    #5
  6. Mark P Guest

    Howard wrote:
    > <> wrote in message
    > news:...
    >> 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.
     
    Mark P, Aug 24, 2006
    #6
  7. Steve Pope Guest

    Noah Roberts <> wrote:

    > 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
     
    Steve Pope, Aug 24, 2006
    #7
  8. Noah Roberts wrote:
    > 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
     
    Clark S. Cox III, Aug 24, 2006
    #8
  9. David Harmon Guest

    On 23 Aug 2006 13:57:17 -0700 in comp.lang.c++,
    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.
     
    David Harmon, Aug 24, 2006
    #9
  10. 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
     
    Frederick Gotham, Aug 24, 2006
    #10
  11. Frederick Gotham wrote:
    > 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
    --
    Please remove capital 'A's when replying by e-mail
    I do not respond to top-posted replies, please don't ask
     
    Victor Bazarov, Aug 24, 2006
    #11
  12. 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).

    --

    Frederick Gotham
     
    Frederick Gotham, Aug 24, 2006
    #12
  13. Guest

    Howard wrote:
    > 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
     
    , Aug 24, 2006
    #13
  14. Guest

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

    Cindy

    Frederick Gotham wrote:
    > 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).
    >
    > --
    >
    > Frederick Gotham
     
    , Aug 24, 2006
    #14
  15. Guest

    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


    David Harmon wrote:
    > On 23 Aug 2006 13:57:17 -0700 in comp.lang.c++,
    > 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.
     
    , Aug 24, 2006
    #15
  16. red floyd Guest

    red floyd, Aug 24, 2006
    #16
  17. 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.

    --

    Frederick Gotham
     
    Frederick Gotham, Aug 24, 2006
    #17
  18. Howard Guest

    "Frederick Gotham" <> wrote in message
    news:cemHg.13055$...
    > 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.
    >


    Geez... how about keeping your hatred of Microsoft out of this?
    -H
     
    Howard, Aug 24, 2006
    #18
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Replies:
    0
    Views:
    900
  2. Mr. SweatyFinger
    Replies:
    2
    Views:
    2,076
    Smokey Grindel
    Dec 2, 2006
  3. Gene

    Re: Bad code or bad compiler?

    Gene, Jun 1, 2008, in forum: C Programming
    Replies:
    3
    Views:
    286
    Barry Schwarz
    Jun 1, 2008
  4. Willem

    Re: Bad code or bad compiler?

    Willem, Jun 1, 2008, in forum: C Programming
    Replies:
    9
    Views:
    285
    Keith Thompson
    Jun 2, 2008
  5. rantingrick
    Replies:
    44
    Views:
    1,250
    Peter Pearson
    Jul 13, 2010
Loading...

Share This Page