2

Backstory (You can skip)

I am building an API for managing Files and Directories in a consistent manner across a project. This is for deduplication and consistency when performing a task, and in this particular case, I want to ensure that a file is closed and the mutex is unlocked. Now this is a straight-forward task to solve if I simply create a new object QByteArray to hold the value while I clean up, but I would like to know if it is actually possible to forgo this, and return with cleanup code happening regardless.

Problem:

Take the following FUNCTIONAL code:

QByteArray  Foo::getFileContents(QCD::FileSystem fileSystem, QString fileName) 
{
    formatDirectoryPath(fileSystem, fileName);
    QString key(d_ROOT.absoluteFilePath(fileName));
    m_Mutex.lock();
    ct_Check(!m_Files.contains(key));       // QMap<QString,QFile*> m_Files
    ct_Check(!m_Files[key]->exists());
    ct_Check(!m_Files[key]->open(QIODevice::ReadOnly));
    QByteArray ba(m_Files[key]->readAll()); // I dont want to create a new object
    m_Files[key]->close();                  // Needs to run before return
    m_Mutex.unlock();                       // Need to free the mutex
    return ba;
}

As you can see, I had to create a QByteArray object to hold my value while I closed the file. It would be nice if I could just do this instead:

QByteArray  Foo::getFileContents(QCD::FileSystem fileSystem, QString fileName) 
{
    formatDirectoryPath(fileSystem, fileName);
    QString key(d_ROOT.absoluteFilePath(fileName));
    m_Mutex.lock();
    ct_Check(!m_Files.contains(key));
    ct_Check(!m_Files[key]->exists());
    ct_Check(!m_Files[key]->open(QIODevice::ReadOnly));
    return m_Files[key]->readAll();
    && m_Files[key]->close(); // Illegal, but you get the idea
    && m_Mutex.unlock();     
}

where I return the readAll() but still manage somehow to close the file inside the function block as well as unlock the mutex.

Is this possible without having to create any more objects?

  • 2
    Your code has no extra objects, because of copy elision (which is guaranteed since C++17). It just has an extra variable. – amon Feb 5 at 14:35
  • 2
    Why dont you use RAII? For the mutex you can directly use QMutexLocker, which will unlock the mutex in its destructor. You easily implement a similar class that closes a QFile in its destructor. – pschill Feb 5 at 15:30
  • 3
    @Akiva You dont unlock the mutex in the QMutex destructor. You unlock it in the QMutexLocker destructor. I recommend that you familiarize yourself with RAII, which is a mandatory concept for C++ developers. It even has its own page at cppreference and coincidentally uses mutexes as example: en.cppreference.com/w/cpp/language/raii – pschill Feb 5 at 16:17
  • 1
    Why not just pass in a QByteArray reference owned by the caller? The actual method can do all the mutex/file maintenance it wants and return an error code or exception on failures. – Patrick Hughes Feb 5 at 18:40
  • 1
    @pschill QFile closes itself in it's destructor. QFile::open and QFile::close are basically vestigial at this point, they can be replaced by QFile::QFile and QFile::~QFile – Caleth Feb 5 at 20:18
11

Addressing the question about cleanup code:

  • yes, there is a way,
  • it's called RAII (for Resource Acquisition Is Initialization, an acronym whose limpid clarity is rivalled only by CRTP), and
  • it is both idiomatic and highly recommended (eg, by the C++ Core Guidelines)

You'll see there is built-in support for RAII for some common types in the form of std::unique_ptr and std::unique_lock.

You can obviously roll your own wrappers for custom types (or for Qt types if necessary, although it looks like std::unique_lock<QMutex> would probably work fine, and QMutexLocker is provided anyway).

You can even implement things like commit-or-rollback if you need something more elaborate.

Your specific code would most straightforwardly translate to

struct FileCloser {
    static void operator()(QFile *file) {
        file->close();
    }
};

QByteArray  Foo::getFileContents(QCD::FileSystem fileSystem,
                                 QString fileName) 
{
    formatDirectoryPath(fileSystem, fileName);
    QString key(d_ROOT.absoluteFilePath(fileName));

    // lock mutex on construction
    QMutexLocker lock(&m_Mutex);

    ct_Check(!m_Files.contains(key));
    ct_Check(!m_Files[key]->exists());
    ct_Check(!m_Files[key]->open(QIODevice::ReadOnly));

    std::unique_ptr<QFile, FileCloser> closer(m_Files[key]);

    return m_Files[key]->readAll();

    // NB. these destructors are guaranteed to execute
    // in the correct order shown:
    // m_Files[key]->close(); by closer dtor
    // m_Mutex.unlock();      by lock dtor
}

Slightly nicer would be to write a utility function to open the file for you, returning that unique_ptr. Then you can't accidentally insert another line in between acquiring the resource and initializing the object to own it.


Is this possible without having to create any more objects?

Technically no, but I'd argue these objects are vanishingly small and inexpensive.

  • Technically, what you've done with the mutex lock is RAII, but the file closer is RRID. Your "Slightly nicer" paragraph explains the key distinction. – Adrian McCarthy Feb 7 at 19:12
4

I guess what you really want is to avoid unnecessary copying of the array. In case you are using a C++ compiler which cannot optimize this automatically by "copy elision" (as mentioned by @amon in a comment), you can avoid the copying by using pointers, preferably smart pointers, like this:

std::shared_ptr<QByteArray> Foo::getFileContents(QCD::FileSystem fileSystem, QString fileName) 
{
    formatDirectoryPath(fileSystem, fileName);
    QString key(d_ROOT.absoluteFilePath(fileName));
    m_Mutex.lock();
    ct_Check(!m_Files.contains(key));       // QMap<QString,QFile*> m_Files
    ct_Check(!m_Files[key]->exists());
    ct_Check(!m_Files[key]->open(QIODevice::ReadOnly));
    auto ba = std::make_shared<QByteArray>(m_Files[key]->readAll()); 
    m_Files[key]->close();
    m_Mutex.unlock();
    return ba;
}

(if you prefer QSharedPointer in favor of std::shared_ptr, I am sure you can use it in a similar manner). Of course, that's a little bit more ugly than your original variant, but it should actually do what you are after.

Another option without pointers/smart pointers is to let the caller pass a reference to an empty QByteArray, and fill that using append:

void Foo::getFileContents(QByteArray &baResult, QCD::FileSystem fileSystem, QString fileName) 
{
    formatDirectoryPath(fileSystem, fileName);
    QString key(d_ROOT.absoluteFilePath(fileName));
    m_Mutex.lock();
    ct_Check(!m_Files.contains(key));       // QMap<QString,QFile*> m_Files
    ct_Check(!m_Files[key]->exists());
    ct_Check(!m_Files[key]->open(QIODevice::ReadOnly));
    baResult.append(m_Files[key]->readAll()); 
    m_Files[key]->close();
    m_Mutex.unlock();
}
  • This was my first thought, too. Both ideas are simple, straightforward, and not tricksy so the next programmer to work with this system can figure out what's supposed to happen easily. – Patrick Hughes Feb 5 at 18:43
1

Since it seems in your case that m_Mutex and m_Files are not locally scoped to the function, no, this is not possible. A function call is really just a labelled piece of instructions that get jumped to and from when called and returning, any code after the return statement is logically not reachable by the cpu. Most likely, your compiler would even notice that this code is unreachable and either produce a warning or simply optimize it away completely.

However

If m_Mutex and m_Files were locally scoped and had their cleanup as part of their destructors, this would be possible since the objects would be destroyed as part of unwinding the stack after the function call. Just beware the return type of readAll in that case, since if the return is a pointer to something inside your m_Files, that pointer would be invalidated.

  • 1
    Debatable, but the point of the answer was to elucidate on post-return cleanup more than what is and isn't in itself useful in a destructor. – Jimmy Holm Feb 5 at 15:07

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.