Inspecting the code I think I've found a problem, or at least an
unnecessary complication.
Firstly, Allocator_Zeroed is only suitable for allocating certain types,
since it creates a new instance and then zeroes its bytes.
This is used to allocate objects of type MYSQL and initialise them to
zero. This makes assumptions about struct MYSQL and is not portable if
the struct contains a pointer, since there's no guarantee that the NULL
pointer is represented by an all-zero bit pattern (although on most
real-world systems it is). I think the only _truly_ portable way to
initialise a pointer is by assinging 0 to it.
This code shows how that allocator breaks a class' invariants:
#include <cassert>
#include "mysqlcppapi/smartpointer/SharedPtr.h"
struct Five
{
const int i;
Five() : i(5) {}
};
using namespace mysqlcppapi;
int main()
{
SharedPtr<Five, Allocator_Zeroed<Five> > sp;
assert( sp->i == 5 );
}
This problem stems from the fact that a default constructed SharedPtr
contains a default constructed pointee, rather than being NULL. This
requirement means that the pointee type must be default constructable
and must not mind being zeroed if Allocator_Zeroed is used.
Allocator_Connection uses Allocator_Zeroed, and so contains the above
problem. Since the zeroed MYSQL structure is passed immediately to
mysql_init() (which initialises it correctly) there is no danger of
using the zeroed (incorrectly initialised) structure. Therefore it's
not wrong, per se - but it smells a bit. I would prefer not to have a
template that is only safe to use for POD types.
I believe this could be avoided by making Allocator_Connection
not use Allocator_Zeroed at all, but let mysql_init() allocate the
structure. This would mean that mysql_close() must be called
unconditionally in deallocate(), so the structure will be deallocated.
This would greatly simplify the SharedPtr code by removing the 2-stage
allocate+initialise combination, and therefore remove the need
for the Do2ndStageDeallocation feature.
deallocate() would call mysql_close() whether the connection was
opened or not (which is correct, since it must free the memory that
mysql_init() reserved).
The attached patch shows basically what I mean, but is not complete (*)
Comments, thoughts?
jon
(*) the SharedPtr class still stores the do2ndStageDeallocation member,
it just doesn't pass it to the allocator's deallocate() function ...
... changing that would mean the Connection can't tell if it's
connected, but that's easy to change by adding a new shared_ptr<bool>
in Connection ...
... but since Collection is basically an aggregation of several
shared_ptr objects we could simplify it further by making Collection
just a wrapper around a single shared_ptr<Collection::Impl> and make
Collection::Impl hold actual types
e.g.
class Collection {
// ...
class Impl; // fwd decl
shared_ptr<Impl> m_pimpl;
};
class Collection::Impl {
// ...
std::string unixsocketstr;
int port;
int timeout;
bool locked;
bool success;
bool connected;
};
but this is all for later discussion.
--
To conquer oneself is a greater task than conquering others.
- Buddha
|
