Hi,
I've got a long hate relationship with this warning, because it kept
popping up and never showed a real problem in my code. Some more
objective arguments against can be found inline.
Nikolai Pretzell wrote:
> warning:
test.cxx(18) : warning C4355: 'this' : used in base member initializer
list
class A
{
public:
A() {}
};
class B
{
A& _a;
public:
B( A& a ) : _a(a) {}
};
class C : public A
{
B _b;
public:
C() : _b( *(A*)this ) {}
BTW: Why this cast? Surely using the implicit conversion is fine. And if
you insist on pointing out what happens, use static_cast.
};
int main()
{
C c;
}
Using 'this' in the ctor initializer list can be dangerous, if you
don't know what you do. The above example uses a safe way to provide
this.
This example does not look too safe to me, because:
It is quite possible that - for example - the maintainer of B one day
wants to use a virtual funtion of A that is implemented in C.
There are no virtual functions in A that can be abused :-o [That
probably means the inheritance should not be public, but that is a
different issue].
But this points out that there is only a problem:
1. if A has virtual interfaces that B might want to use
2a. if B does a lot of work during construction,
which might involve calling members of its argument OR
2b. the constructor of C uses B in ways,
which might make B call members of the constructor argument
3. if B is maintained independently of A
In my experience passing 'this' to member or base class constructors is
most often done with classes that are designed for this purpose so it is
part of their contract that you can do that. Often these classes are
designed for close collaboration and so are not even maintained
independently.
And that functionality of B may be called in the constructor body of C.
Which highlights that use of 'this' in the constructor body, where more
complex operations may happen, is at least as dangerous as using it in
the initializer list. And it is subject to the same problems, for
example if C is not the most derived class.
And IMHO this condition (2b above) is very specific. The rule would be:
"don't pass 'this' to a subobject and then do complex things with that
subobject in your constructor". But there is no warning for this rule
(which is good advice), but for a much wider range of cases including
perfectly safe ones.
And btw: In the example C does not call members of this->_b in the
constructor body.
Pro warning:
In cases like this, generally the maintainer of B has no chance to know
that he must not use some functions of A (or C), because the instance of
A (or C) is not yet completely constructed.
I think it's the reverse. The author of C should not do this unless this
way of using it is part of the design of B. But if it is, this use is a
valid use case. And it is not easy to replace by a safer usage pattern.
So the warning is a valid hint to a potential maintainability nightmare.
Relying on behaviors that are not documented is a common cause of
maintainability problems. In general you cannot warn about them. I don't
find this case to be so special, that it warrants a compile error.
Contra warning:
The normal workaround would be to move the "this" from the
initialization section into the constructor body. This removes the
warning, but not always removes the danger, because still the receiver
of "this" may use it while not all construction is done.
In order to be able to move use of 'this' to the constructor body, the
object being initialized needs to support a two-stage initialization:
first default-initialize to an invalid state and then finish
initialization through a member function. During a init() function I'm
much more inclined to assume that all my members are fully usable. So
doing only this may even increase the risks. So now you'd have to add
two-stage initialization to C as well. But even that does not solve the
problem that C:init() -> B:init() -> C:member() might invoke C
frunctionality before C is ready.
In many cases two-stage initialization creates more problems than it
solves, because now a class has a weaker invariant and needs to check
whether it is valid all the time and clients must be able to handle the
errors reported by an invalid instance.
So one could argue, fixing the warning does not necessarily fix the
problem, so we can remove the warning anyway.
Right. The warning (with warnings=errors) forces us to use a workaround,
which may even have a negative impact on the design (and thus
maintainability). And this workaround avoids one potentially risky
practice by using an equally risky one which happens to be not warned about.
The one thing that the warning does, it that it makes you think about
this risk when you encounter it. But once you have become accustomed to
simply use the workaround design from the outset, that wears off
although the problem still lingers.
I think we have two possible ways to solve this problem.
- Turn of the warning for every occurrence in the code.
- Switch of C4355 for Windows globally.
I'd suggest to take the warning serious and to re-look on each
occurrence about this way:
1. Can the construction be re-worked so no self-reference is necessary?
Sometimes this is easy by clarifying a hierarchy that could be a
directed graph but currently is a non-directed one.
If yes, do so.
2. If not, is the use of "this" safe including maintainenance in later
times?
If yes, see X.
3. If no, make sure there is a comment at all places, using the
partly-initialized instance that it may be only partly initialized!
Then goto X.
X. See, if moving "this" from initialization section into c'tor body
does any harm. (In case of dialogues, the painting takes so much time,
that one could assume, the minor performance drawback of moving the
initialisation into the body is negligible - but I don't know that code,
so it's just an assumption.)
I don't know about dialogs, but in general the ability to move this into
the c'tor body requires a redesign of the classes used. For example a
reference meber may need to become a pointer and suddenly you have to
check for NULL on all uses.
If yes, do so.
If no, disable the warning locally.
I find this ugly. And if we go this we we should look for a way to use a
platform independent, readable way to disable it inline with the code
(e.g. a SAFE_USE_OF_THIS_IN_CTOR_BEGIN macro) instead of sprinkling
platform-specific stuff all over the code (or makefiles).
If we just generally disable the warning, we may overlook occurrences
where the use indeed is questionable.
As mentioned before, I don't think this case is special enough to
warrant all this overhead when equally dangerous, similar cases are left
without warning.
There can be a coding/code review rule about self-referentiality and use
of 'this' in a constructor context and maybe another one about one- vs.
two-stage (de)initialization. But as the warning cannot check the entire
rule, it may even be detrimental, because it creates a false sense of
security (the compiler detects violations of this, so I don't need to
check myself).
--
Joerg Barfurth Sun Microsystems - Desktop - Hamburg
>>>>>>>>>>>>>>>>>> using std::disclaimer <<<<<<<<<<<<<<<<<<<<<<<
Software Engineer [EMAIL PROTECTED]
Thin Client Software
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
