January 25, 2006
Hi,

This really should go in a C++ newsgroup, but I've noticed that I have more in common with the people here than on a C++ group. Must be all the frustrations with C/C++ and the hope for a better tomorrow. More than hope, an actual vision ; )

Here's a construct I can't seem to get right in C++. It works, but I hate the way I've implemented it. Maybe somebody has a better idea?

It starts like this: I convert some handle into a class to make use of C++'s RAII, ctor/dtor. I do this all the time, but today it happens to be for sqlite3_get_table / sqlite3_free_table. Needless to say, the handle (char**) obtained by sqlite3_get_table must be freed by sqlite3_free_table. The wrapper for this table handle I've called SqliteRS (from Result Set):

class SqliteRS
{
  char** table;
  unsigned int cursor, rows, columns;
public:
  SqliteRS(char** t, int c, int r) : table(t), cursor(c), columns(c),
rows(r) {}
  ~SqliteRS() { sqlite3_free_table(table); }
....
};

And you obtain an SqliteRS by calling Exec of the class SqliteDB, which wraps the database handle, sqlite3* (error handling omitted):

SqliteRS SqliteDB::Exec( const char* zSql )
{
  sqlite3_get_table(pdb, zSql, &t, &r, &c, NULL );
  return SqliteRS(t,c,r);
}

All this to be able to do:

SqliteRS results = database.Exec("select * from X");

And everything should be freed nicely.
The problem of course is that the temporary return value will actually be
copied into the variable "results", causing the table to be freed when the
temporary return value gets destructed, leaving "results" with an invalid
handle. This is the problem I'm trying to solve.

It's noteworthy that Visual Studio optimizes this code, even in debug mode! There won't be a temporary return value that gets copied to the actual variable. However, it won't compile if I create a private copy-ctor, eventhough it's never called when it's public.

At the moment I have two ways around this:

1) create a copy ctor that steals the pointer (sets it to NULL in the
temporary return value); or
2) create a seperate class for the return value and a ctor that takes this
class in SqliteRS.

1) would be implemented like this:

SqliteRS::SqliteRS( const SqliteRS &st )
{
  memcpy(this,&st,sizeof st);
  const_cast<SqliteRS&>(st).table = NULL;
}

2) is a bit messier, but basically goes like this: SqliteDB::Exec returns an _SqliteRS with only the basic information (char**,int,int in this case). _SqliteRS has not dtor. SqliteRS becomes a ctor that takes an _SqliteRS, and the dtor that frees the handle. For added DBC one could add an assertion in the _SqliteRS dtor to assert that it was in fact copied into an SqliteRS, but this would require a const_cast<> similar to 1).

Returning handles from other functions / factories happens only too often, and wrapping these handles in a class to make use of RAII (ctor/dtor) seems the way to go. But why is this contruction so tricky to do nicely? Am I forgetting something?

In D the class will be created on the heap and you're returning the handle of the class, so there's never a copy involved. One can even use RAII:

class SqliteDB
{
....
  SqliteRS Exec( char[] sql ) { .... return new SqliteRS(...); }
}

auto SqliteRS result = database.Exec("...");

This works just fine.

Lionello.

By the way, has somebody else noticed that I can put an auto variable (the RAII kind) in a global handle and it still gets destructed when the 'auto' leaves the scope? Shouldn't the compiler complain?


January 25, 2006
Lionello Lunesu wrote:
> Hi,
> 
> This really should go in a C++ newsgroup, but I've noticed that I have more in common with the people here than on a C++ group. Must be all the frustrations with C/C++ and the hope for a better tomorrow. More than hope, an actual vision ; )
> 
> Here's a construct I can't seem to get right in C++. It works, but I hate the way I've implemented it. Maybe somebody has a better idea?
> 
> It starts like this: I convert some handle into a class to make use of C++'s RAII, ctor/dtor. I do this all the time, but today it happens to be for sqlite3_get_table / sqlite3_free_table. Needless to say, the handle (char**) obtained by sqlite3_get_table must be freed by sqlite3_free_table. The wrapper for this table handle I've called SqliteRS (from Result Set):
> 
> class SqliteRS
> {
>   char** table;
>   unsigned int cursor, rows, columns;
> public:
>   SqliteRS(char** t, int c, int r) : table(t), cursor(c), columns(c), rows(r) {}
>   ~SqliteRS() { sqlite3_free_table(table); }
> ....
> };
> 
> And you obtain an SqliteRS by calling Exec of the class SqliteDB, which wraps the database handle, sqlite3* (error handling omitted):
> 
> SqliteRS SqliteDB::Exec( const char* zSql )
> {
>   sqlite3_get_table(pdb, zSql, &t, &r, &c, NULL );
>   return SqliteRS(t,c,r);
> }
> 
> All this to be able to do:
> 
> SqliteRS results = database.Exec("select * from X");
> 
> And everything should be freed nicely.
> The problem of course is that the temporary return value will actually be copied into the variable "results", causing the table to be freed when the temporary return value gets destructed, leaving "results" with an invalid handle. This is the problem I'm trying to solve.

> At the moment I have two ways around this:
> 
> 1) create a copy ctor that steals the pointer (sets it to NULL in the temporary return value); or
> 2) create a seperate class for the return value and a ctor that takes this class in SqliteRS.

[snip]

> Returning handles from other functions / factories happens only too often, and wrapping these handles in a class to make use of RAII (ctor/dtor) seems the way to go. But why is this contruction so tricky to do nicely? Am I forgetting something?

This is what std::auto_ptr<> is perfect for. An auto_ptr will, using your words, steal the reference being assigned to it. A auto_ptr is perfect when you want to transfer responsibility.

> In D the class will be created on the heap and you're returning the handle of the class, so there's never a copy involved. One can even use RAII:
> 
> class SqliteDB
> {
> ....
>   SqliteRS Exec( char[] sql ) { .... return new SqliteRS(...); }
> }
> 
> auto SqliteRS result = database.Exec("...");

Yes, but with this, the user needs to remember to make the reference auto. There is (AFAIK) no way in D to make this automatic like the c++ auto_ptr does. If D had more power (assignment overloading, struct destructors/constructors), an auto_ptr would be straight-forward to implement in D too.

/Oskar
January 25, 2006
>> auto SqliteRS result = database.Exec("...");
>
> Yes, but with this, the user needs to remember to make the reference auto. There is (AFAIK) no way in D to make this automatic like the c++ auto_ptr does. If D had more power (assignment overloading, struct destructors/constructors), an auto_ptr would be straight-forward to implement in D too.

Maybe the prototype of the Exec function could already mention the 'auto'?

auto SqliteRS Exec( char[] sql ) { ... }

At the moment the compiler (correctly) complains with "functions cannot be const or auto".

This kind of "auto" could also be used to force the caller to catch the return value in a variable. I guess exceptions can be used to prevent the caller from ignoring the result value.

Exec("select * from X"); // not allowed; use ExecNonQuery

Lio.


January 25, 2006
Lionello Lunesu wrote:
> 
> Returning handles from other functions / factories happens only too often, and wrapping these handles in a class to make use of RAII (ctor/dtor) seems the way to go. But why is this contruction so tricky to do nicely? Am I forgetting something?

A lot of it has to do with the messed-up way the ODBC API was written--it just doesn't map well into an object model.  What I've done in the past is store the handles in an RAII-type object as you are:

class db_handle_ptr :
private	noncopyable
{
public:
	SQLHDBC	hdbc;
	db_handle_ptr() : hdbc( SQL_NULL_HDBC ) {}
	~db_handle_ptr() {
            ::SQLDisconnect( hdbc );
            ::SQLFreeHandle( SQL_HANDLE_DBC, hdbc );
        }
};

And then pass around references to it using a shared pointer (similar to the one available in Boost).  I get around returning handles at all by instead returning a temporary object that is only accessible by the destination object:

class resultset :
private	noncopyable
{
public:
    class resultset_init
    {
    public:
        resultset_init( shared_ptr<db_handle_ptr> const& hdb,
                        shared_ptr<stmt_handle_ptr> const& hs );

    private:
                friend resultset;

                shared_ptr<db_handle_ptr>    m_db;
                shared_ptr<stmt_handle_ptr>  m_stmt;
    };

    resultset( resultset_init const& init );

private:
    shared_ptr<db_handle_ptr>   m_db;
    shared_ptr<stmt_handle_ptr> m_stmt;
};

class statement :
private noncopyable
{
public:
    resultset::resultset_init open();
};

The end result is that all the handles that need to stay alive do so until all dependent objects are destroyed.  So even if the connection object is deleted, any open statements or resultsets will remain usable.


Sean
January 25, 2006
Oskar Linde wrote:
> 
> This is what std::auto_ptr<> is perfect for. An auto_ptr will, using your words, steal the reference being assigned to it. A auto_ptr is perfect when you want to transfer responsibility.

Yes, but it's not perfect if you wish to simply store a reference to something as a class member, because auto_ptr performs a destructive copy.  Just something to be aware of for those who haven't used auto_ptr before.


Sean
January 27, 2006
When I think about it....

Shouldn't an ignored return value be treated as an error?

Or at least a warning, or an assertion (like forgetting to return something). Maybe adding a construction like:

void = I_dont_care_about_the_return_value();

L.


January 27, 2006
Lionello Lunesu wrote:
> When I think about it....
> 
> Shouldn't an ignored return value be treated as an error?
> 
> Or at least a warning, or an assertion (like forgetting to return something). Maybe adding a construction like:
> 
> void = I_dont_care_about_the_return_value();
> 
> L. 

The problem is, not all return values matter.
There are lots of APIs which have some really stupid return values. The classic example being printf().

void = printf("hello world\n");

No thanks.
I think you'd need some kind of "must use" syntax on a function declaration, which would trigger an error if it wasn't used. Maybe a "nonvoid" keyword.

But I don't think this is a very common programming error. Not really worth worrying about.
January 27, 2006
> void = printf("hello world\n");

You shouldn't be using printf to begin with : S

But I agree, not a common mistake. I was thinking about the waste of CPU time when somebody discards return codes. Fits nicely with D's the notion of "statement must have side effect", for example a "{strlen(a);}" compiles fine, but is a waste of time.

I made the following template when I noticed that many colleagues were ignoring the return value of some Lock() call:

//! Assert that a (return) value gets used (used for handles)
template <typename T>
class _use
{
  const T retval;
  bool unused_return_value;
public:
  _use( const T& rv ) : retval(rv), unused_return_value(false) {}
  ~_use() { assert(unused_return_value); }
  operator T () { unused_return_value=true; return retval; }
};

I defined the call as _use<bool> Lock() and it now asserts when the return value is not caught into some variable or expression. I know, I should have used C++ exceptions, which I hope to do when we get more time on our hands... (probably never :S)

Lio.


January 27, 2006
Lionello Lunesu wrote:
> 
> I made the following template when I noticed that many colleagues were ignoring the return value of some Lock() call:
> 
> //! Assert that a (return) value gets used (used for handles)
> template <typename T>
> class _use
> {
>   const T retval;
>   bool unused_return_value;
> public:
>   _use( const T& rv ) : retval(rv), unused_return_value(false) {}
>   ~_use() { assert(unused_return_value); }
>   operator T () { unused_return_value=true; return retval; }
> };
> 
> I defined the call as _use<bool> Lock() and it now asserts when the return value is not caught into some variable or expression. 


I know, I should have
> used C++ exceptions, which I hope to do when we get more time on our hands... (probably never :S)

I'm not sure about that. This is a logical error, and ideally it would be caught at compile time. I think an assert is the next best option.
(Because even if the exception is caught, there's still a logic error).

I'm a little biased against exceptions, because the worst bug I've ever encountered in my life was caused by a destructor which threw an exception, inside a multi-threaded program. (There were virtually no symptoms, that single thread just silently disappeared - yikes).
January 27, 2006
Don Clugston wrote:
> 
> I'm a little biased against exceptions, because the worst bug I've ever encountered in my life was caused by a destructor which threw an exception, inside a multi-threaded program. (There were virtually no symptoms, that single thread just silently disappeared - yikes).

That in itself is basically an illegal operation in C++, since throwing an exception while another is in flight terminates the program.  If in doubt, wrap the contents of your dtor in a try{}catch(...){} block, and report the error by some other means.  I'll admit I do like that D doesn't have this problem, even if it means that some exceptions are overridden by others and are therefore never reported.  I've wondered whether exception chaining via the .next property might be right for this purpose, but it's not a perfect fit.


Sean
« First   ‹ Prev
1 2
Top | Discussion index | About this forum | D home