Return instance causes 'attempting to reference a deleted function' error

Question

After working with C# for the past decade or two, my C++ is getting a little rusty.

I'm writing a database class and have an issue with the following method:

CRecordset CAccessDatabaseReader::ExecuteSqlQuery(LPCTSTR pszSqlQuery)
{
    CRecordset recordSet(&m_Database);
    recordSet.Open(CRecordset::forwardOnly, pszSqlQuery);
    return CRecordset(recordSet);
}

The compiler complains on the line with the return statement:

Error C2280 'CRecordset::CRecordset(const CRecordset &)': attempting to reference a deleted function

Can someone help me understand exactly what is happening here?

CRecordSet appears to be a non-copyable class. It does not have a copy constructor. — Sam Varshavchik, Dec 07 '16 at 01:26
In C++, you can turn off copying of a class, and that is what CRecordset has done. That's why you can't return it by value. — PaulMcKenzie, Dec 07 '16 at 01:40
To "return" a CObject-based class like this you'll probably have to use dynamic allocation. — M.M, Dec 07 '16 at 01:51

score 1 · Answer 1 · edited May 23 '17 at 12:30

1

CRecordset has deleted its copy constructor, so you cannot return it by value. You could possibly return a std::unique_ptr<CRecordset> or a reference from ExecuteSqlQuery instead.

On deleting copy constructors: https://stackoverflow.com/a/6077164/2449857

On returning references from functions: Is the practice of returning a C++ reference variable, evil?

edited May 23 '17 at 12:30

Community

1
1

answered Dec 07 '16 at 01:48

Jack Deeth

3,062
3
24
39

There is no "returning a reference" here – M.M Dec 07 '16 at 01:52
@M.M He said he's writing the `CAccessDatabaseReader` class. So he can make `ExecuteSqlQuery` return a `CRecordset&` or `const CRecordset&` if he does it right. – Jack Deeth Dec 07 '16 at 01:54
1

That would be a bad idea. The `unique_ptr` example in the answer on that thread might be useful though. – M.M Dec 07 '16 at 01:54

Remy Lebeau · Accepted Answer · 2016-12-08T18:40:56.387

CRecordset's copy constructor has been explicitly marked as deleted to prevent copying CRecordset objects from one to the other.

So, the function will have to return a new object by pointer and require the caller to delete the object when finished using it:

CRecordset* CAccessDatabaseReader::ExecuteSqlQuery(LPCTSTR pszSqlQuery)
{
    CRecordset *recordSet = new CRecordset(&m_Database);
    if (!recordSet->Open(CRecordset::forwardOnly, pszSqlQuery))
    {
        delete recordSet;
        return NULL; // or raise an exception
    }
    return recordSet;
}

CRecordset *rs = reader.ExecuteSqlQuery(TEXT("..."));
if (rs)
{
    ...
    delete rs;
}

Or better:

std::unique_ptr<CRecordset> CAccessDatabaseReader::ExecuteSqlQuery(LPCTSTR pszSqlQuery)
{
    std::unique_ptr<CRecordset> recordSet(new CRecordset(&m_Database));
    if (!recordSet->Open(CRecordset::forwardOnly, pszSqlQuery))
        recordSet.reset(); // or raise an exception
    return recordSet;
}

std::unique_ptr<CRecordset> rs = reader.ExecuteSqlQuery(TEXT("..."));
if (rs)
{
    ...
}

Can I ask what is the point of returning `std::unique_ptr<>`? Why not just return a regular pointer, and then have the caller assign that pointer to a variable of type `std::unique_ptr<>`? Isn't that simpler somehow? — Jonathan Wood, Dec 08 '16 at 18:19
And if the caller doesn't assign the raw pointer to a smart pointer class? Then the caller risks a memory leak if it forgets to `delete` the pointer, or an uncaught exception occurs before it calls `delete`. The function has no way of knowing what the caller is or is not going to use. So better to be explicit about what memory management is expected. The whole point of using smart pointers is to avoid these kinds of issues. This also helps the function itself auto-manage the memory in case something goes wrong before `return` is called. — Remy Lebeau, Dec 08 '16 at 18:38

Return instance causes 'attempting to reference a deleted function' error

2 Answers2