[fdo-internals] Incorrect implementation of noncopyable concept

[Pierre Dalcourt]

Hi Mateusz,

 

I agree that ideally we should enforce non-copiability in all our
non-copiable classes (across all providers), and that boost provides a
nice shorthand way to achieve this.

 

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.
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".

 

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.

 

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 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. :-)

 

Pierre

 

 

-----Original Message-----
From: fdo-internals-bounces at lists.osgeo.org
[mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Mateusz
Loskot
Sent: February 1, 2007 7:28 PM
To: fdo-internals
Subject: [fdo-internals] Incorrect implementation of noncopyable concept

 

Hi,

 

Recently, I've been searching through sources of the ArcSDE provider

(http://svn.osgeo.org/svn/fdoarcsde/) and I found there is used

concept of noncopyable types.

I'd like to report a bug here - as we don't have bug tracker up and

running yet (or I don't know where it is) - because noncopyability

is misused here

In the provider, all is based on declaring public assignment operator,

but not implementing it intentionally. For example here:

 

http://svn.osgeo.org/svn/fdoarcsde/trunk/Providers/ArcSDE/Src/Provider/A
rcSDEInsertCommand.h

 

This approach is insufficient to make a type noncopyable, because I can

still call:

 

A a;

A b(a); // assignment operator is not used

 

but current solution prevents only this case:

 

A a

B b;

b = a; // assignment operator is used, but linker will complain

 

1st issue, is that there is missing copy constructor.

Copy constructor makes (mostly) inseparable pair together with

assignment operator, so they are need to be used together.

 

Let's assume there is a copy constructor added.

 

2nd issue, is that the noncopyable concept is based on declaring copy

ctor and assignment op as private, to cause compiler errors but not

linker errors. Compiler errors are meaningful here ie. saying "can not

access private copy ctor" but linker error is mostly meaningless.

 

Full version of noncopyable concept is implemented as follows:

 

class A

{

public:

    A() {}

private:

    // Blocked copyability - DO NOT IMPLEMENT

    A(A const&);

    A& operator=(A const&);

};

 

int main()

{

    A a;

    A b(a); // can not access private A::A()

    A c;

    c = b;  // can not access priavate A::operator=()

 

    return 0;

}

 

 

Obviously, it's easy to forgot about putting private prototypes for

every noncopyable class, so IMHO it's better to use noncopyable

concept already available from Boost:

 

#include <boost/utility.hpp>

 

class A : boost::noncopyable

{

public:

    A() {}

};

 

IMO, it's better because shorter and doesn't bloat a class definition.

Also, note, the inheritance is intentionally *private*, so the relation

between A and noncopyable types is HAS-A (composition)

but not IS-A (inheritance).

 

Cheers

-- 

Mateusz Loskot

http://mateusz.loskot.net

_______________________________________________

fdo-internals mailing list

fdo-internals at lists.osgeo.org

http://lists.osgeo.org/mailman/listinfo/fdo-internals

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.osgeo.org/pipermail/fdo-internals/attachments/20070202/2547d346/attachment-0001.html