[Scummvm-devel] The Curious Case of NonCopyable

[Matt]

Hello,

In doing some cleanups of warnings reported by G++'s -Weffc++ and PC-Lint, 
I came across this kind of warning:

lint-undeserved-173x.cpp:11: warning: 'class PluginManager' has pointer 
data members
lint-undeserved-173x.cpp:11: warning:   but does not override 
'PluginManager(const PluginManager&)'
lint-undeserved-173x.cpp:11: warning:   or 'operator=(const 
PluginManager&)'

PluginManager (indirectly) inherits from NonCopyable, which defines a 
private copy ctor and assignment operator that do not have an 
implementation. If anyone ever tried to use either, they'll get a link 
error. This is the desired effect, and helps avoid a whole host of pointer 
aliasing bugs.

However, the checks in these tools basically say it doesn't count. This is 
because the pointer member is in the subclass, which the NonCopyable 
parent can't possibly know about and therefore handle the copy (deep or 
otherwise).

Also, the indirect nature of this might be confusing to some 
folks who would add a copy ctor or assignment operator in the subclass. 
With -Wall and without -Wextra *and* -Weffc++, there is no warning that 
something has gone wrong. (Unless, of course you explicitly enable the 
individual warnings to catch these specific issues, which I don't think is 
possible.) This means that these protections could be easily circumvented 
by accident, leaving the safety net of code reviews and/or manual testing 
to find the issue.

To remedy these issues, I suggest eliminating NonCopyable as a "mix-in" 
type, and favoring a macro to be used directly in the subclasses that 
should not be copyable. This takes the declarative approach closer to the 
class is actually affects, making it more explicit and a little easier to 
understand. It also eliminates  these warnings and allows future warnings 
to highlight real potential problems.

I suggest a macro that looks like:

#define PREVENT_COPYING_FROM(klass)\
     type(const klass&);\
     void operator=(const klass&)

I can make this change locally and commit it as part of my warning 
cleanup. If that is agreeable, where should the macro go?


--
tangled strands of DNA explain the way that I behave.
http://www.clock.org/~matt