isocpp
C.82 please recommend alternative to virtual function calls in constructor/destructor
C.82: Don’t call virtual functions in constructors and destructors Just to be clear up front. I understand that virtual dispatch in a constructor is not a valid "alternative" because it simply doesn't work.
The guidelines identify two cases where an call to a virtual function is acceptable
- If the function is final and therefore virtual and non-virtual dispatch will have the same effect
- If the function is explicitly qualified so it's not actually a virtual call at all
As such in the scenario where I encounter or want to write code that violates this rule I have these options:
- mark the function final
- call the function explicitly
- extract the functionality of the function into a separate non-virtual function which the virtual function calls
My question is which is the preferred solution?
That's the question. My proposed answer is to combine 1 and 2 so that it's obvious to anyone reading the calling code and anyone hoping to override the function what is going on. If the function can't be final because it's overridden later then my instinct is to use 3. However I think 1 is usually a valid option because the average inheritance heirarchy isn't very deep in my experience (often just a interface with a handful of concrete implementations).
2 seems the obvious alternative to me. It is simple, straight forward and a method that is localized to the point where the problem arises. Making a function final and using it in constructor are completely orthogonal concerns and should not be mixed, so that removes 1. 3 Would be an ok choice too, but it introduces an additional function unnecessarily.
Thanks for your response. I'm not yet convinced but that's exactly the kind of concrete recommendation I'd like from this rule.
The justification I see for 1 is that as somebody implementing a child which overrides the method in question I want to be able to assume that I'm changing the behavior everywhere the method is called. I think 2 makes it very clear at the call site that something unusual is going on but not at the place where the method is overridden. Keeping in mind as well that it's not just about the original child class author but also anyone reading the child or parent class later.
I think a design in which it is expected that a child class would override a method but we're only going to use that override sometimes is unexpected. If they're not going to override it (option 1) then it's fine. If we split the method into the virtual bit and the non-virtual bit (option 3) then it's fine. Option 2 by itself either relies on the child class implementer to be knowledgeable and diligent enough to check how the function is used (which I think is optimistic) or requires the parent class implementer to be able to assert that the discrepancy between which version of the method gets called will result in correct behavior for any given child class (which I think is naive).
That's my current thinking but I could be convinced otherwise I think.
Obviously you don't expect (or should not expect) an overriden virtual function to affect the base classe's constructor, so I don't see why that matters. If the correctness of what you are doing in your constructor depends on a virtual function not being overriden in a derived class, then I'd say that is a ~~red herring~~ warning sign pointing to a bad design. Maybe you could provide a concrete (code) example of what you are concerned about.
EDIT: Fixed wrong idiom
I suspect a lot of the time you run into this problem it probably is an indicator of bad design and as such it's a little tricky to produce example code where the solution isn't just fix the design but here goes.
Consider something like this.
class IResetable
{
virtual void Reset() = 0;
};
class Parent : IResetable
{
Parent() { Reset(); }
void Reset() override
{
// Do a bunch of work to put the object into some well defined initial state
}
};
class Child : Parent
{
void Reset() override
{
Parent::Reset(); //probably
//Do a bunch more stuff to put the object into some well defined initial state
}
};
It violates C.82 and won't do what you want it to or what it looks like it should do. You can make it sort of do what it's supposed to by calling Reset from Child's constructor (though that's not perfect because Parent::Reset gets run twice). You can also make the behavior explicit by calling reset qualified. However it seems like option 3 is the right solution to me.
What if child doesn't exist so you have
class IResetable
{
virtual void Reset() = 0;
};
class Parent : IResetable
{
Parent() { Parent::Reset(); }
void Reset() override
{
// Do a bunch of work to put the object into some well defined initial state
}
};
This is basically option 2 only. This is fine as long as you're not expecting Child to exist or not expecting Child::Reset to exist or not expecting Child::Reset to need to be called on construction or expecting Child to make sure Child::Reset is called. By marking the method as final you make sure Child::Reset doesn't exist.
This is basically option 2 only. This is fine as long as you're not expecting Child to exist or not expecting Child::Reset to exist or not expecting Child::Reset to need to be called on construction or expecting Child to make sure Child::Reset is called.
I completely disagree: If it is fine at all, then it is fine. Period.
It does exactly what I should expect it to do when I read the code (namely, whatever Parent::Reset() is doing). If the constructor is supposed to do something different than what Parent::Reset() does under any circumstances, you should not call Parent::Reset() unconditionally. Simple as that. At this point it doesn't make any difference, if Reset is a virtual function or not.
Also,you would be exactly in the same situation, if you chose option 3:
class Parent : IResetable
{
Parent() { _reset_impl(); }
void Reset() override
{
_reset_impl()
}
void _reset_impl()
{
// Do a bunch of work to put the object into some well defined initial state
}
};
You have exactly the same behavior as with solution 2, but now with a more complicated implementation. For that matter - you are actually in the exact same boat, if you'd neither call Reset, nor _reset_impl, but implement the Constructor and Reset independently from each other.
class Parent : IResetable
{
Parent() {
// Do a bunch of work to put the object into some well defined initial state
}
void Reset() override
{
// Do the exact same thing as in the constructor
}
};
By marking the method as final you make sure Child::Reset doesn't exist.
And how is that situation better. Your class (and especially the constructor) should not care if Child::Reset exists, because it isn't calling Child::Reset? All you'd have achieved is that a potential Child classes state will no longer be resettted, if IResetable_ptr->Reset() is called, because it can't override the Reset function anymore, so why allow inheritance from Parent at all?
If you have a good reason to prevent overriding a particular function in your child class, then mark it final, yes. That you want to call the function inside your constructor isn't a good reason however.
If it is fine at all, then it is fine. Period.
I agree. My assertion is that therefore it is not fine.
You're right that the reading of Parent::Reset() is unambiguous. My concern is not with the call site but rather with the hypothetical child implementation site. I think it is a reasonable expectation when the child is implementing Reset that all places that call Reset will use that function. However that's not true if the call is explicitly qualified (or indeed if it's called from a constructor/destructor regardless). The implementer of the Child class doesn't know how the function is called from the Parent so can't know whether it will be used inconsistently. Similarly the parent doesn't know what will be added to Child::Reset and so whether it is applicable in the places where it eschews it.
It's possibly worth noting that the call sites don't always obviously originate in a constructor/destructor so if ctor calls A calls B calls C calls Parent::Reset() then that affects everything that calls A/B/C regardless of whether it's from a virtual dispatch possible context.
I'm not claiming there's any ambiguity here. Explicit qualification exists and so anyone overriding a function should know that the parent class could choose to totally ignore the new behavior when calling the function. I just think it's an added complexity that would be nice to avoid in this circumstance. Perhaps not nice enough to justify limiting the design of descendant classes.
Another way to think about it is that the method documentation on Reset would probably say something like Resets the object to its original state or something. This would be true when calling Reset() but not when calling Parent::Reset(). As such when you call Parent::Reset there is no documentation that describes what is actually taking place. This isn't insurmountable but it's a weirdness that's avoided if Parent::Reset is simply equivalent to Reset because the method can't be overridden.
All you'd have achieved is that a potential Child classes state will no longer be resettted
That is true but my expectation was that the point that you need to modify the child code to include resetting you would change the parent.
My assertion is that therefore it is not fine.
If it is not fine to call Reset() in the constructor, then why are we discussing how to do it properly?
I think it is a reasonable expectation when the child is implementing Reset that all places that call Reset will use that function.
For one: No it isn't, because we just dicussed to instances where this is not happening (in a qualified function call and inside of constructor/destructor)
Second: The implementor of a child class should not be concerned about whether or not Reset is called in the constructor of the parent or not. What matters is the documented behavior of that constructor and it doesn't matter if the author implements that behavior by calling Parent::Reset(), Parent::_reset_impl() , copy pasting the code from Parent::Reset() or by completely differnt means - that is an implementation detail that should not concern you.
Now, maybe you have a different mental model of virtual functions or what override means or you had to deal with different bugs and maybe even design patterns in your carrier than I, but in any case, I think we reached the point where we just have to agree to disagree.
That is true but my expectation was that the point that you need to modify the child code to include resetting you would change the parent.
Sorry, I think there is a typo or two in this sentence, because I'm unable to understand it (however, I'm not a native speaker, so the problem might be on my side)
No you've convinced me with this:
No it isn't, because we just dicussed to instances where this is not happening (in a qualified function call and inside of constructor/destructor)
Basically my thinking was that by marking the function as final you avoid future authors having to worry about that case but actually that's just small part of a larger set of considerations so you're not really solving the problem.
Thanks for talking through it with me.
With this in mind it sounds like the rule should say that if you want to call code in a virtual function you should explicitly qualify it. It almost says this already so maybe the editors will feel that this is already implied.