[Scummvm-devel] The Curious Case of NonCopyable

[Matt]

On Tue, 28 Apr 2009, Max Horn wrote:

>> 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).
>
> Frankly I don't quite understand what you are saying here. So, those
> tools say something, fine -- yet I have seen tons of lint tools
> outputting spurious and incorrect warnings, so... What is the actual
> *issue* you perceive here? Can you give a code example that does
> something bad / wrong in an unexpected fashion?

First, let me remind that I know a thing or two about automated code 
analysis. Your tone implies (to me) I am blindly reading these things and 
parroting them without much though. This is not the case.

I did give an example where the protections could easily be circumvented 
by accident, and the compiler would not warn. You didn't quote that part; 
did you read it before you replied?


> In particular, trying to make a copy of a PluginManager does cause a
> compile time error (and the class is never to be subclassed, being a
> singleton). The only way I know of to defeat the purpose of
> NonCopyable is to make a subclass which provides its own explicit copy
> constructor / assignment operator -- well, if you try hard enough, you
> can always shoot into your own foot ;-).

The thing that makes theis error-prone is that the "attribute" of 
NonCopyable is three hops away in the inheritance hierarchy, in this case. 
That doesn't express the intention very well, and often leads to 
inadvertent mistakes, in my experience. Expressing the intention in the 
subclass reduces/eliminates this potential.

On top of all of this, this is just an inappropriate use of inheritance. 
The name "NonCopyable" implies a capability, which should be implemented 
as a pure interface. The copy ctor and assignment operator are not pure 
interfaces, for obvious reasons. As such, this stops being a "mix-in" 
style of multiply-inheriting different pure interfaces and sets an example 
for the classic multiple-inheritance anti-patterns that we all know are 
bad. Just in case the "bad" isn't clear: First, because this isn't a pure 
interface, we expect the subclasses to be a specialization of the 
NonCopyable base class -- they are not. Second, because of the multiple 
inheritance of concrete classes going on, if one of those other base 
classes does have these operators defined, any number of build/link issues 
could occur.

In short, it sets a bad example of C++ and OO programming for people who 
aren't deeply familiar with either or both. Part of mistake-proofing is 
making sure that if a part of the code will be used as an example or 
template, that it isn't propagating an anti-pattern.

I hope this is clear enough, now :)


> All in all, I am not convinced there is actually an issue, but then
> again, I am not sure I understood what your point is (it didn't come
> across from what you wrote, at least). So, I'll wait for some more
> explanations from you, resp. an example :-).

I provided an example in the original email, verbally. An example of 
another issue like this that is lying in wait is here:

--- Module:   gui\GuiManager.cpp (C++)
                                         _
                                 _data = new byte[_size];
graphics\cursorman.h(181) : Info 1732: new in constructor for class 
'Palette'
     which has no assignment operator
graphics\cursorman.h(181) : Info 1733: new in constructor for class 
'Palette'
     which has no copy constructor
                                 _
                         _data = new byte[_size];
graphics\cursorman.h(151) : Info 1732: new in constructor for class 
'Cursor'
     which has no assignment operator
graphics\cursorman.h(151) : Info 1733: new in constructor for class 
'Cursor'
     which has no copy constructor


Now, if this ever did result in a pointer aliasing issue at runtime, 
valgrind could find it. Assuming that you covered the codepath that 
exhibits the problem at runtime, which implies a high amount of code 
coverage that is repeatable and not time-intensive. In my experience, both 
runtime and static analysis tools need to be leveraged to get the best out 
of both.

Note that I am *not* just running the tool and bugging developers to 
eliminate warnings. I'm eliminating warnings myself, as it's something I 
can do in the small windows of time I have in my busy schedule. I have a 
lot of experience with this particular tool, and know how to balance its 
strengths and weaknesses. I fixed several other issues that had no 
ambiguity whatsoever; this issue, however, seemed to need more discussion 
to come to an agreed-upon solution.

Also note that this same issue is reported by G++ wth -Weffc++ and/or 
-Wextra. So, even if you have an allergic reaction to indepdendent code 
analysis tools, this is at least one place that an open source tool is in 
(correct) agreement with the proprietary tool.

I hope this helps! :)

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