[fdo-internals] Incorrect implementation of noncopyable concept

[Mateusz Loskot]

Pierre Dalcourt wrote:
> However, as the code currently stands (the copy constructor is not
> explicitly declared private), you will not be able to use it (at least
> not outside the class itself).  This is because the destructor is
> declared private and you cannot instantiate an object unless you have
> access to both the constructor and destructor of that object's class. 

Pierre,

Not exactly.
Copy operation has nothing to do with availability of dctor.
You can have dctor declared as private and you can still make a copy
through initialization or even assignment, unless you block copying:

class A
{
private:
   ~A(){}
};

A* a = new A();
A* b = new A(*a);

A* c = new A();
c->operator=(*b);

> So if you try your scenario below, you will get an error "cannot access
> private member declared in class 'ArcSDEInsertCommand' [...] see
> declaration of ArcSDEInsertCommand::~ArcSDEInsertCommand".

I gave the ArcSDEInsertCommand as an example.
If you scan the sources, you will find this copyable stuff messed

and inconsistent, some types block assignment op, some block
assignment op and copy ctor, some of them declare dctor as private,
but some as public (ie. ArcSDEFeatureReader).

If there was no element of blocking copy operation partially
implemented, I wouldn't raise this subject. I'm not analysing the
provider for how well is copyability implemented or not.
I've just noticed some inconsistency, potentially dangerous.

> Also, since we don’t export these methods (using __declspec(dllexport)),
> they are not accessible to users outside the provider dll on Windows.
> I realize that this is not the case for Linux, but the FDO API is designed
> so that you rarely should be accessing provider-specific classes anyway
> (notable exceptions are for provider-specific schema override classes,
> and provider-specific commands).  So the kind of scenario you describe
> would typically be limited to the code within the shared library.

You're right. I just consider a provider developer as a user of a
provider source code too.

> Lastly, even if we do protect the copy constructors across the board,
> users could still copy an object if they really wanted to, since there
> are always ways around C++’s access mechanisms:
>  
> 
>   ArcSDEInsertCommand* source_object =
> (ArcSDEInsertCommand*)mConnection->CreateCommand(FdoCommandType_Insert);
> 
>   ArcSDEInsertCommand* dest_object =
> (ArcSDEInsertCommand*)malloc(sizeof(*ins));
> 
>   memcpy(dest_object, source_object, sizeof(*source_object));
> 
>   //Can now use dest_object even though constructors/destructors are private

I can not agree.
The code above is C++ ill-formed and represents undefined
behaviour (UB).

The ArcSDEInsertCommand has:
- non-trivial constructor
- destructor
- base class
- data members of non-POD type

So, according to current C++98 (things are going to slightly change in
new C++0x standard) the class ArcSDEInsertCommand is not a POD type.
Using (according the C++98) memcpy() to copy non-POD type is as UB, so
it's not guaranteed to work, nor to be portable.

Certainly, most implementations of C++ won't raise any disaster
when memcpy is used on non-POD types, *but* on non-POD types which
have the same byte layout as POD equivalent - this is not the case in
FDO commands because these types are far from POD.

Not to mentione that copying complex FDO types like those discussed
here won't preserve the internal state of object well (ie. no tick of
reference counter, etc.).

> I guess what I’m saying is that it’s not always worth trying to prevent
> people from shooting themselves in the foot because if they really want
> to then they’ll find a way. :-)

I guess, if someone did define private operator=() then he has some idea
in mind (to make a type noncopyable?), but the work sees to be not
finished...

Cheers
-- 
Mateusz Loskot
http://mateusz.loskot.net