2

Say I have an RAII class, instances of which should never be copied:

class Session {

public:
    Session(); // Allocates a resource and sets generates a unique Session::id. 
    ~Session(); // Frees the resource

    Session(const Session&) = delete;
    Session& operator = (Session&) = delete;

private:
  std::uint32_t id;
}

Given that I can't allow copies, I'd like to allow moving, so I need to implement the move constructor and move-assignment operator but I'm not sure what I should do with Session::id of the moved instance.

I can either:

  1. Set it to some value known to be invalid (maybe change the type to a signed int and use -1 as the invalid value)
  2. Use something like std::optional and set it to std::nullopt to invalidate it.

Which (if either) of those options is correct?

Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
John O'brien
  • 321
  • 1
  • 9
  • you can also just set a `bool`. – apple apple May 31 '23 at 20:12
  • That decision is part of the semantics of your program. It also depends on how you want to optimize the code. The `std::optional` will waste 4 bytes for the boolean member in it. I'd say go for the `std::optional` approach if it makes the code more idiomatic and easier to interact with (for the client). – digito_evo May 31 '23 at 20:14
  • 1
    Go with `std::optional`, unless the logic creating `id` already has a return value indicating an error. Most libs with a C interface would come with such a return value and `if(id != InvalidId)` is probably a bit easier to read than `if (!id.has_value())`... – fabian May 31 '23 at 20:15
  • @appleapple If you mean adding a `bool` member to the class then that's essentially what a single `std::optional` member does under the hood. And yet an `optional` member does it more effectively. – digito_evo May 31 '23 at 20:23
  • hmm, I like @appleapple suggestion, as the boolean field will be a single-purpose, dedicated field to indicate whether the class is valid. Whereas the std::optional will be multi-purpose (it holds a thing **and** indicates whether the instance is valid). The boolean field would be better if I had more fields, as then I wouldn't have to pick which one to make multi-purpose and wrap in an std::optional. Hmmm – John O'brien May 31 '23 at 20:26
  • @digito_evo afaict it cannot be more effective (assume you mean speed) than simply set a flag. – apple apple May 31 '23 at 20:36
  • @appleapple performance wise they're most probably the same. But in terms of design and concise code, optional should be better. – digito_evo May 31 '23 at 20:40
  • @digito_evo I don't think so. especially for basic (raw resource) RAII wrapper (which OP is doing) – apple apple May 31 '23 at 20:42
  • 1
    fwiw I'm actually against use `std::optional` here as there is nothing really **optional** – apple apple May 31 '23 at 20:45
  • 3
    side note: the parameter of the copy assignment operator should be `const`, just like in the copy constructor, eg: `Session& operator = (const Session&) = delete;` – Remy Lebeau May 31 '23 at 20:48
  • What is the purpose of session id? Unless the id is directly related to the ressource, I would use an unique id per object (if useful for debugging for example) or no id at all. – Phil1970 May 31 '23 at 22:05
  • I'm sorry for asking such an opinion-based question. I'm unsure of which answer to accept, as they're all excellent and have their own advantages. I'm probably going to go with the boolean member field for now, as that's the most readable (for me) and clearly communicates intent. Thank you all for your time! – John O'brien May 31 '23 at 22:36
  • @JohnO'brien You better accept one of the answers if it helped the most since that's the convention here on SO and it also will help the future visitors. – digito_evo Jun 01 '23 at 10:42
  • `std::unique_ptr` might be an option... – Jarod42 Jun 01 '23 at 12:30

3 Answers3

6

Since this can be done without std::optional by just sacrificing one value out of the 232 possible ids, I'd do like std::string::npos and initialize a constant of the unsigned type your id has with -1.

Example:

class Session {
public:
    // the constant used to signal an invalid id:
    static constexpr std::uint32_t invalid_id = static_cast<std::uint32_t>(-1);

    Session() :
        id(id_counter++)
        /* allocate the resource */
    {}
    Session(const Session&) = delete;
    Session(Session&& other) noexcept :
        id(std::exchange(other.id, invalid_id)) // exchange with the invalid id
        /* move or exchange the resource */
    {}
    Session& operator=(const Session&) = delete;
    Session& operator=(Session&& other) noexcept {
        // check for self-assignment if needed
        std::swap(id, other.id);
        // swap the resource
        return *this;
    }
    ~Session() {
        if(id != invalid_id) { // if the check is even needed
            /* free the resource */
        }
    }

private:
    inline static std::uint32_t id_counter = 0;
    std::uint32_t id;
};

Demo

An option would be to let 0 be the invalid_id if that would feel more natural in your case. You'd just have to initialize invalid_id with 0 and change the id counting to id(++id_counter).

Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108
2

std::optional is a poor fit because a moved from optional is not disengaged. (example)

Because its default move behavior doesn't match the desired semantics, one would still have to provide custom move operations and the std::optional would not be of much value.

One could write an optional-like type which has the desired move operations. One advantage of this approach is that one could specify the default unused value as a template parameter. One disadvantage of this approach is that classes have more than one member and they are likely all valid or invalid. Therefore, using an optional-like type, after correcting move operations, will lead to moving all class state to another type. That is quite a burden to impose.

On the other hand, one could easily write a flag-like class that provides the indication of whether the instance has been moved from.

class engaged {
public:
    constexpr engaged() noexcept = default;
    constexpr engaged(const engaged&) noexcept = default;
    constexpr engaged& operator=(const engaged&) noexcept = default;
    constexpr engaged(engaged&& other) noexcept : engaged_(std::exchange(other.engaged_, false)) { }
    constexpr engaged& operator=(engaged&& other) noexcept { engaged_ = std::exchange(other.engaged_, false); return *this; }
    constexpr explicit operator bool() const noexcept { return engaged_; }
private:
    bool engaged_ = true;
};

(example)

How would you do it?

There are trade-offs. An RAII type typically has more than a single integer. There are also usually not many such types. It may not matter whether it has been moved from. Generally speaking, I would focus on which RAII types to write and the appropriate interface for each.

Jeff Garrett
  • 5,863
  • 1
  • 13
  • 12
1

I would only choose the std::optional approach if the class had a single member. So in this case, having a bool member for indicating the validity of an instance of the class seems fine.

Using the bool member you can implement an overloaded operator bool() for the class. This way, expressions like if (session_instance) are possible (instead of writing if (session_instance.id != -1) or if (session_instance.id.has_value()), etc).

An example:

#include <utility>
#include <cstdint>
#include <cstddef>
#include <cstdio>

class Session {

public:
    explicit Session(const std::uint32_t id, const std::size_t size = 10)
    : is_valid { true }, m_id { id }, m_buffer { new char[size] }
    {
        m_buffer[size - 1] = '\0';
    }
    ~Session()
    {
        if ( m_buffer != nullptr )
            delete[] m_buffer;
    }

    Session(const Session&) = delete;
    Session& operator = (const Session&) = delete;

    Session(Session&& rhs) noexcept
    : is_valid { std::exchange(rhs.is_valid, false) },
      m_id { std::exchange(rhs.m_id, 0) },
      m_buffer { std::exchange(rhs.m_buffer, nullptr) }
    {}
    Session& operator = (Session&& rhs) noexcept
    {
        if ( this != &rhs )
        {
            is_valid = std::exchange(rhs.is_valid, false);
            m_id = std::exchange(rhs.m_id, 0);
            if ( m_buffer != nullptr )
                delete[] m_buffer;
            m_buffer = std::exchange(rhs.m_buffer, nullptr);
        }

        return *this;
    }

    operator bool () const noexcept
    {
        return is_valid;
    }

private:
    bool is_valid;
    std::uint32_t m_id;
    char* m_buffer { nullptr };
};

int main( )
{
    Session ses { 1000, 10 };
    Session ses2 { 100, 20 };
    ses2 = std::move( ses );

    if ( ses )
        std::printf( "ses is valid\n");
    else
        std::printf( "ses is NOT valid\n");
}

Note: The operator bool() can also be implemented with return m_id != -1; in case of changing the type of m_id to int. Then no extra bool member will be required. You can simply add a member variable to your class like inline static constexpr int invalid_id { -1 }; and compare m_id with it inside the operator bool().

digito_evo
  • 3,216
  • 2
  • 14
  • 42
  • 1
    _"instead of writing `if (session_instance.id != -1)`"_ - would be in `operator bool () const noexcept { return id != invalid_id; }` if that operator is needed though, so the interface to the user wouldn't be different in that regard. – Ted Lyngmo May 31 '23 at 21:49
  • @TedLyngmo Yes, right. Gonna edit it. – digito_evo May 31 '23 at 21:57
  • 2
    Another nitpick: `if ( m_buffer != nullptr ) delete[] m_buffer;` should be just `delete[] m_buffer;` since `delete[]` already makes this `nullptr` check internally. – Ted Lyngmo May 31 '23 at 22:03