Freigeben über

POP QUIZ: What’s wrong with this code – part 2

  • Artikel

Here is another snippet of code for you to look at and tell me what is wrong.  As before, give your comment of what is wrong and I will post the answer and make the comments public tomorrow.  This time it is C++ code and not specific to ASP.NET:

 typedef bool (FUNC)(void *);

class Foo
{
    virtual FUNC Method;

    void *m_p;
};

bool Foo::Method(void *p)
{
    return  p == m_p;
};

Comments

  • Anonymous
    May 14, 2008
    A bunch of things come to mind:
  1. there's no implementation of Method returning FUNC, (supposing it is an (wrong) attempt to declare a method);
  2. everything is implicitly private so virtual doesn't make sense;
  3. if the intention is to have an implementation that is virtual, it is not good style to touch private variables. You will probably want to make m_p protected in that case;
  4. m_p is never initialized and will have a random value in a release build (or worse, it'll disclose a previous value of whatever used to be in memory);
  5. Method is never initialized (see 4). This one is worse as invoking it would result in arbitrary code being executed; -- Henkk

  • Anonymous
    May 15, 2008
    The declaration specifies a function pointer, and your implementation provides a function.

  • Anonymous
    May 15, 2008
    PingBack from http://blogs.msdn.com/tom/archive/2008/05/15/pop-quiz-what-s-wrong-with-this-code-part-2.aspx

  • Anonymous
    May 15, 2008
    typedef bool (FUNC)(void *); should be typedef bool (*FUNC)(void *); no? You're using typedef to define a pointer for a function, and not the function itself.

  • Anonymous
    May 15, 2008
    Well, im not too sure what are you getting at.  I am assuming you are trying to check if the address of the two pointers is the same.  But I'm pretty sure that won't work on void pointers and you will have to type cast them first.

  • Anonymous
    May 15, 2008

  1. Method is virtual, yet access is private. A child can not use the default implemention. Furthermore, as m_p is also private, the child can't provide  an implementation that uses m_p.
  2. m_p has not been set anywhere, yet it is being compared with.
  • Anonymous
    May 15, 2008
    The only mistake I found in a glance was in the last statement return p == m_p; No comparison operator can be used in this manner esp in a return statement Correct me if I am wrong