We are wrapping an object implementing an abstract class IFunctionality, in a class we are writing that also implements IFunctionality.
IFunctionality interface is defined in third party code and at the moment it only consists of virtual functions, most of them being pure virtual. Non-pure virtual functions usually have an empty implementation and are overridden in the specific implementation. IFunctionality does not have any member variable (at the moment).
Our wrapper looks like this:
class WrapperFunctionality : public IFunctionality
{
public:
WrapperFunctionality(IFunctionality& pOriginal)
: m_pOriginal(pOriginal)
{
}
// We override all virtual functions and forward them to the original.
void doX() override { m_pOriginal->doX(); }
void doY() override { m_pOriginal->doY(); }
// ...
// Well, except a few functions that we specialize.
// That's why we need the wrapper.
void doSomethingSpecial() override { ... }
};
All is fine at the moment. But the code could break silently in the future.
Issue 1: thirdparty code could add new non-pure virtual functions to IFunctionality. It would get undetected and we would fail to call the according function in the original object.
Issue 2: thirdpary code could add public members to IFunctionality and could directly access these members. We would also fail to forward these modifications to the original objects.
I wonder whether we could detect these issues at compile time with the help of static_assert or some template magic.
Issue 2 has been discussed in How to detect if a class has member variables? without a satisfying solution. But in my thinking, it is less likely to happen (as these third-party developers don't seem to prefer public member variables for this interface class anyway).
But issue 1 is more likely to happen. The interface class already has non-pure virtual functions and is likely to get new ones. Is there a way to enforce that my class implements all virtual functions (pure or not) from that class?
To clarify things, let me give a more concrete description of the the situation.
The interface represents a surface on which one can draw geometry with given attributes:
class ISurface
{
public:
virtual void setColor(const RGB& color) = 0;
virtual void setOpacity(float alpha) = 0;
virtual void drawLine(...) = 0;
virtual void drawCircle(...) = 0;
// etc
};
A specific ISurface implementation is instantiated by the third party framework, and there are multiple implementations of it based on the back-end.
We have many objects in our system that know how to draw themselves to an ISurface. We don't control their draw function. The objects can be from third party plugins:
class IDrawableObject
{
public:
virtual void draw(ISurface* pSurface) const = 0;
};
Then a possible implementation of IDrawableObject, possibly from third party code:
class SomeObject : public IDrawableObject
{
public:
virtual void draw(ISurface* pSurface) const override
{
pSurface->setColor(RGB(255, 0, 0));
pSurface->drawLine(...);
pSurface->setOpacity(0.7f);
pSurface->fillCircle(...);
}
};
At some point, we want to change the visual representation of these third party objects by overriding their color or opacity. We can hook into the procedure that makes the draw call of the object:
// That function will be called by the framework
virtual void drawObjectToSurface(const IDrawableObject* pObject, ISurface* pSurface) override
{
pSurface->setColor(m_overrideColor);
pSurface->setOpacity(m_overrideAlpha);
// Next line does not work as expected, because the object
// overwrites our color and alpha before drawing itself
//pObject->draw(pSurface); // does not work
// Instead, we use a wrapper that blocks color and opacity writes
BlockingSurfaceWrapper wrapperSurface(pSurface);
pObject->draw(&wrapperSurface);
}
The only solution that won't require runtime overhead or veritable hacks and similar mayhem is to write a libclang analysis pass that verifies that all base virtual methods got overridden, and then run this pass as a part of the build. There are numerous online examples of various static analyses implemented with libclang. That's the only solution that will scale, and as soon as you get that working, you'll be well set to add various other analyses to your build process, to enforce other policies or design requirements. In the long run, that's probably the only way to go.
I wouldn't bother with any other hacks in the interim. Before static analaysis is implemented, you should add the check for this to your manual code review checklist, and it will be caught by humans reviewing code about to be committed/merged to the repo. If you don't have a code review checklist, or worse yet - don't review merge requests - it's high time you got it going.
It is extremely suspicious that you treat code changes as if they were some destructive force majeure that you're powerless against. It hints that you don't have a code review process, and are thus living exposed to the whims of the weather. That's how you get frostbite, and that's how eventually projects fail. The problem you have shared with us is just a small one from a large list of other problems you get in a bundle when code can be changed without a second pair of eyes looking at it, with no rules surrounding merging changes.
Reviews will address all your concerns, just in case it wasn't clear:
thirdparty code could add new non-pure virtual functions to IFunctionality - the third party code should be either a submodule in your git repo, or copied into the repo. Any changes to the third party code must be a part of the code review process will get caught by the checklist items.
thirdparty code could add public members to IFunctionality and could directly access these members - ditto, code review will catch that.
Now you ask: Isn't that crazy to review third party code? No. It's not, because you obviously depend on that code so tightly, that it's functionally an integral part of your code base - that's what happens when the interfaces are poorly designed. It's not merely a dependency. You have to treat it as any other code you wrote for your project.
Nothing in life is free, and of course the reviews will take time - the more items on the checklist, the more of a drag they'll be. An empty checklist won't mean that you can get rid of reviews: the checklist should be kept down to the very essentials, otherwise no human will retain sanity while ensuring compliance. But now you see that you have options: you can spend time on longer code reviews, or you can spend time learning to use libclang. It's a real game changer - just over a decade ago, industry-strength static code analysis "building block" libraries were only available for lots of money, or you had to pay a company specializing in code analaysis to implement your analysis as a black-box using the tooling they developed to offer such services.
It's either extending the code reviews or implementing the static analysis. No way around it.