C++ ctor question

[Pix]

Trying to get rid of some errors in someone else's old C++ code.

Consider:

class Base
{
public:
int i_;
Base(int i) : i_(i) {};
};

class Derived
{
public:
int j_;
Derived(int i, int j) : Base(i), j_(j) {} ;
};

Visual C++ objects:
warning C4355: 'this' : used in base member initializer list

and the help says:

The this pointer is valid only within nonstatic member functions. It cannot be used in the initializer list for a base class.

The base-class constructors and class member constructors are called before this constructor. In effect, you've passed a pointer to an unconstructed object to another constructor. If those other constructors access any members or call member functions on this, the result will be undefined. You should not use the this pointer until all construction has completed.

The following sample generates C4355:

Copy Code
// C4355.cpp
// compile with: /W1 /c
#include <tchar.h>

class CDerived;
class CBase {
public:
CBase(CDerived *derived): m_pDerived(derived) {};
~CBase();
virtual void function() = 0;

CDerived * m_pDerived;
};

class CDerived : public CBase {
public:
CDerived() : CBase(this) {}; // C4355 "this" used in derived c'tor
virtual void function() {};
};

CBase::~CBase() {
m_pDerived -> function();
}

int main() {
CDerived myDerived;
}

I think this will wind up being OK as long as none of Derived's members attempt to use any of Base's members, but I'd like to get rid of the compiler warning, as it's in a header file that's used in 11 source files, and generates a couple of dozen warnings. IIRC, class initializers like this happen before the constructor. That may or may not matter. Wouldn't

Derived(int i, int j)
{
i_ = i;
j_ = j;
}

be correct as long as there are no oddball side effects (which I believe is the case?)

[RonHiler]

Wouldn't

Derived(int i, int j)
{
i_ = i;
j_ = j;
}

be correct as long as there are no oddball side effects (which I believe is the case?)

Yes. Generally this is the only way I will do it. I dislike initialization lists (I find them hard to read while scanning code) in constructors. There is a case where an initialization list is required (won't work any other way), but you haven't hit it here.

[Pix]

I actually like initialization lists, but the guy who originally wrote this code was in love with 'em:

GameWindow::GameWindow()
:view_thread_id(0), init_thread_id(0), simulate_thread_id(0),
ps_window( NULL ), map( new Map ), viewwin_( NULL ),
statusbar_( NULL ), infoarea_( NULL ), buttonbar_( NULL ),
frame_( NULL ), palette( NULL ), horiz( NULL ), vert( NULL ),
toolmsg_obs( toolmsg, closure(this,&GameWindow::notify_update) ),
world_map_( NULL ), menu( NULL )
{
}

[Pix]

Bah, that wasn't the problem after all.

here's the actual code throwing the error, with the problem highlighted.:

class ToggleMenuItem: public MenuItem
{
protected:
    Observer<bool> value_;
public:
    ToggleMenuItem( Window* w, int i, Subject<bool>& v )
        : MenuItem(w,i), value_( v, closure( this, &ToggleMenuItem::notify ) )

closure is a template function. The guy who wrote this code suffered from a desire to be excessively clever.

In any case, I think it's something that may be safe to ignore, at least for now. It may even be a compiler bug, because value_ is a member of the derived class, not the base class, and the initializer for value_ only references members of Derived (I think.)

[RonHiler]

closure is a template function. The guy who wrote this code suffered from a desire to be excessively clever.

Yeesh, you're not kidding. That's a mess. I like using inheritence as much as the next guy, but that's no reason to make the code unreadable. This is the kind of thing that gives the C++ language a bad name. I would certianly not hire anyone that coded like that, no matter how clever they thought they were being

In any case, I think it's something that may be safe to ignore, at least for now. It may even be a compiler bug, because value_ is a member of the derived class, not the base class, and the initializer for value_ only references members of Derived (I think.)

Heh. It's a warning, so it must not crash the program or anything. I don't know that the warning is a bug, the code is, in fact, using the 'this' pointer of the ToggleMenuItem class in the constructor, which at that point could still be undefined (I'm not sure at what point in the constructor 'this' gets defined, before or after the code in the constructor is run).

I could be wrong, but I'm not sure that's defined behavior according to the standard.

I ran into a somewhat similar problem not so long ago with the MD code, as a matter of coincidence. I was using the STL in the destructors (STL strings for logging purposes), and that worked fine in my old compiler, but when I upgraded to the new compiler it began crashing the program. It turns out that in the new compiler the STL had destroyed itself *before* the destructor used it, and thus the blowup. According to the standard, the order of destruction is undefined. I have a feeling you may run into a similar thing there.

[Pix]

I would certianly not hire anyone that coded like that, no matter how clever they thought they were being

ROFLOL. Google did. (Although probably years after he wrote that.)

Heh. It's a warning, so it must not crash the program or anything. I don't know that the warning is a bug, the code is, in fact, using the 'this' pointer of the ToggleMenuItem class in the constructor, which at that point could still be undefined (I'm not sure at what point in the constructor 'this' gets defined, before or after the code in the constructor is run).

Well, I think you have the concept of a partially constructed object. In this case--and this is pure speculation, but it might be interesting to generate the assembly language and see what is actually happening--since the this refers to the derived class, it might be OK; it's just the the object is being built from the outside in.

[RonHiler]

ROFLOL. Google did. (Although probably years after he wrote that.)

Hmm, not surprised. Templates are among the most advanced concepts in C++. If you have the knowledge and ability to properly use them, then it's fair to say you are competent in the language. And generally, that's all that companies look at when they hire.

But being competent in a language is only half of the equation (and, I would argue, not even the most important half, any high school graduate could learn the language to that level in 6 months if they worked at it). But you also have to use that knowledge to write good code.

You know this guy's code better than I do, Pix, obviously I've seen only a small fraction of it, but the bit I've seen is not well written code. I'm sure it works, but I tell you, if it came across my desk, it would be going back to the coder for revision.

I place a great deal of emphasis on readability. It's a pure defensive mechanism for me, heh. I commonly have to scan through thousands of lines of code a day, and I have to do it very quickly, or little gets done (and I've had to go through some pretty craptastic code in my career). Could I read that code that you posted? Sure I could. But it would take me a minute or two to really get a feel for it. Constrast that to well written code that does the same thing that I can scan in 15 seconds, and guess which one I like better? I much prefer a coder with less ability to use templates (for instance) but who writes clean code over one that is technically competent but writes crap (of course, the best scenario is to get one with both advantages, heh, but if I had to chose one over the other, readability would win out every time).

But most companies don't bother with that. They check for language knowledge. It's understandable, you can get a feel for that variable by just sitting them down with a standard quiz and you're done. Checking HOW they write code is a lot harder, and I suspect most HR people aren't going to bother (including, apparently, the ones at Google).

Anyway, sorry for hijacking your question, it's a subject that irks me to some extent