Skip to main content
added 38 characters in body
Source Link
papagaga
  • 5.8k
  • 12
  • 25

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized, local storage.

Even if not, a self-contained object handler probably won't bring anything worth renouncing standard tools such as std::unique_ptr. That you didn't notice that your program would leak memoryresources if the given Functor, or T's constructor, threw an exception, or even if a write isn't read, is a proof of this wise principle: don't reinvent memory management tools.

If you really want to provide your own memory manager, make it a dedicated, RAII based class, and then compose your spsc_object from it and an atomic.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized, local storage.

Even if not, a self-contained object handler probably won't bring anything worth renouncing standard tools such as std::unique_ptr. That you didn't notice that your program would leak memory if the given Functor, or T's constructor, threw an exception, is a proof of this wise principle: don't reinvent memory management tools.

If you really want to provide your own memory manager, make it a dedicated, RAII based class, and then compose your spsc_object from it and an atomic.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized, local storage.

Even if not, a self-contained object handler probably won't bring anything worth renouncing standard tools such as std::unique_ptr. That you didn't notice that your program would leak resources if the given Functor, or T's constructor, threw an exception, or even if a write isn't read, is a proof of this wise principle: don't reinvent memory management tools.

If you really want to provide your own memory manager, make it a dedicated, RAII based class, and then compose your spsc_object from it and an atomic.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};
point out memory leak
Source Link
papagaga
  • 5.8k
  • 12
  • 25

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized, local storage.

Even if not, using uninitialized memorya self-contained object handler probably isn'twon't bring anything worth renouncing standard tools such as std::unique_ptr for resource handling. That you didn't notice that your program would leak memory if the given Functor, or T's constructor, threw an exception, is a proof of this wise principle: don't reinvent memory management tools.

If you really want to provide your own memory manager, make it a dedicated, RAII based class, and then compose your spsc_object from it and an atomic.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized storage.

Even if not, using uninitialized memory probably isn't worth renouncing standard tools such as std::unique_ptr for resource handling. That you didn't notice that your program would leak memory if the given Functor, or T's constructor, threw an exception, is a proof of this wise principle: don't reinvent memory management.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized, local storage.

Even if not, a self-contained object handler probably won't bring anything worth renouncing standard tools such as std::unique_ptr. That you didn't notice that your program would leak memory if the given Functor, or T's constructor, threw an exception, is a proof of this wise principle: don't reinvent memory management tools.

If you really want to provide your own memory manager, make it a dedicated, RAII based class, and then compose your spsc_object from it and an atomic.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};
point out memory leak
Source Link
papagaga
  • 5.8k
  • 12
  • 25

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized storage. 

Even if not, using uninitialized memory probably isn't worth renouncing standard tools such as std::unique_ptr for resource handling. That you didn't notice that your program would leak memory if the given Functor, or T's constructor, threw an exception, is a proof of this wise principle: don't reinvent memory management.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized storage. Even if not, using uninitialized memory probably isn't worth renouncing standard tools such as std::unique_ptr for resource handling.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};

Build on top of the standard library, not along

You didn't specify which version of the standard you target. If it is the most recent one, then you have in std::optional the tool to avoid explicit memory management and keep the benefit of uninitialized storage. 

Even if not, using uninitialized memory probably isn't worth renouncing standard tools such as std::unique_ptr for resource handling. That you didn't notice that your program would leak memory if the given Functor, or T's constructor, threw an exception, is a proof of this wise principle: don't reinvent memory management.

Avoid code duplication

You don't need to have two write methods, and two read methods even less. read already uses a template argument, meaning that Functor& will adapt to either const Functor& or Functor&. For the write method, you can introduce a template argument to get into template deduction context and benefit from a forwarding reference.

Don't clutter variable names with underscores

Classes are meant to qualify names and avoid conflicts, so keep your names informative and beautiful.

With all that in mind, here's what I'd suggest:

#include <type_traits>
#include <atomic>
#include <optional>

template<typename T>
class spsc_object {
    
    std::optional<T> value;
    std::atomic_bool occupied;

public:
    template <typename U>
    bool write(U&& object) {
        static_assert(std::is_convertible_v<U, T>);
        if (!occupied.load(std::memory_order_acquire)) {
            value = std::forward<U>(object);
            occupied.store(true, std::memory_order_release);
            return true;
        }
        return false;
    }

    template<typename Functor>
    bool read(Functor&& functor) {
        if (!occupied.load(std::memory_order_acquire)) 
            return false;
        functor(*value);
        value.reset();
        occupied.store(false, std::memory_order_release);
        return true;
    }
};
Source Link
papagaga
  • 5.8k
  • 12
  • 25
Loading