4
\$\begingroup\$

device_raw_ptr is a simple fancy pointer. It essentially wrap pointers to GPU memory. It's sole purpose is to separate out host pointers from device pointers, i.e. they should not be inter-convertible and must not be dereferenced. At the same time, they should be zero-cost (with respect to raw host pointers) and be maximally compatible with regular pointers.

Helper class:

template <class T>
struct equality_operators {
    /* 
    ** The deriving class must implement the following:
    ** friend bool operator==(const T&, const T&);
    */

    friend bool operator!=(const T& lhs, const T& rhs) { return !static_cast<bool>(lhs == rhs); }
};

template <class T>
struct less_than_operators {
    /* 
    ** The deriving class must implement the following:
    ** friend bool operator<(const T&, const T&);
    */

    friend bool operator>(const T& lhs, const T& rhs)  { return rhs < lhs; }
    friend bool operator<=(const T& lhs, const T& rhs) { return !static_cast<bool>(lhs > rhs); }
    friend bool operator>=(const T& lhs, const T& rhs) { return !static_cast<bool>(lhs < rhs); }
};

template <class T>
struct relational_operators : equality_operators<T>, less_than_operators<T> { };

device_raw_ptr implementation:

template <class T>
    class device_raw_ptr : public relational_operators<T> {
        static_assert(std::is_standard_layout<T>::value, "T must satisfy StandardLayoutType");
    public:
        using element_type = std::remove_extent_t<T>;

        constexpr device_raw_ptr() noexcept = default;
        constexpr device_raw_ptr(std::nullptr_t) noexcept : ptr { nullptr } { }
        constexpr device_raw_ptr(const device_raw_ptr& other) noexcept = default;
        explicit device_raw_ptr(element_type* ptr_) noexcept : ptr{ ptr_ } { }
        device_raw_ptr(device_raw_ptr&& other) noexcept : ptr{ other.ptr } { other.reset(); }

        device_raw_ptr& operator=(const device_raw_ptr& other) noexcept { 
            swap(device_raw_ptr(other), *this); 
            return *this;
        }

        device_raw_ptr& operator=(device_raw_ptr&& other) noexcept { 
            swap(device_raw_ptr(other), *this);
            return *this;
        }

        void reset() noexcept { ptr = nullptr; }
        void reset(T* ptr_) noexcept { ptr = ptr_; }

        element_type* get() noexcept { return ptr; };
        const element_type* get() const noexcept { return ptr; }

        friend void swap(device_raw_ptr& lhs, device_raw_ptr& rhs) noexcept {
            using std::swap;
            std::swap(lhs.ptr, rhs.ptr);
        }

        explicit operator bool() const noexcept { return static_cast<bool>(ptr); }

        device_raw_ptr& operator++() noexcept {
            ++ptr;
            return *this;
        }

        device_raw_ptr operator++(int) noexcept {
            device_raw_ptr tmp(*this);
            ptr++;
            return tmp;
        }

        device_raw_ptr& operator+=(std::ptrdiff_t offset) noexcept {
            ptr += offset;
            return *this;
        }

        device_raw_ptr& operator-=(std::ptrdiff_t offset) noexcept {
            ptr -= offset;
            return *this;
        }

        friend device_raw_ptr& operator+(device_raw_ptr lhs, std::ptrdiff_t offset) noexcept {
            lhs += offset;
            return lhs;
        }

        friend device_raw_ptr& operator-(device_raw_ptr lhs, std::ptrdiff_t offset) noexcept {
            lhs -= offset;
            return lhs;
        }

        /* required by relational_operators base class */
        friend bool operator==(const device_raw_ptr& lhs, const device_raw_ptr& rhs) noexcept { return lhs.ptr == rhs.ptr; }
        friend bool operator<(const device_raw_ptr& lhs, const device_raw_ptr& rhs) noexcept { return lhs.ptr < rhs.ptr; }

    protected:
        T *ptr;
    };

    template <class T, class U, class V>
    std::basic_ostream<U, V>& operator<<(std::basic_ostream<U, V>& os, const device_raw_ptr<T>& other) {
        os << other.get() << " (device)";
        return os;
    }

I am also looking for suggestions on how to order different things inside a class.

Might it be better to initialize with nullptr instead of the default constructor. This breaks compatibility but I think it might be worth considering the compiler would mostly optimize the assignment if it's immediately overwritten.

\$\endgroup\$
  • \$\begingroup\$ I just noticed that I am passing rvalues to swap which accepts non-const lvalue as arguments. You can find it in the copy & move constructor. \$\endgroup\$ – Yashas Apr 7 '19 at 16:56
  • \$\begingroup\$ If anyone did not notice, I messed up the relational_operator inheritance. \$\endgroup\$ – Yashas Apr 8 '19 at 5:14
3
\$\begingroup\$
  1. device_raw_ptr is extremely cheap to copy, so remove all hints of move-semantics and use of swap().

  2. Now you can remove the copy-constructor and copy-assignment, as there is no need to explicitly declare them. Especially making them user-defined must be avoided to keep them trivial.

  3. Kudos on trying to use the approved two-step for swap(). Though you get a failing grade anyway because you bungled it by using a qualified call in the second part. Hopefully, C++20 will abolish that nonsense by introducing customization point objects.

  4. Yes, you should pass your device_raw_ptr by value if you have the choice, as it is a tiny trivial type. Still, refrain from returning a reference to such a temporary.

  5. There is no reason operator-(device_raw_ptr, std::ptrdiff_t) should not be constexpr. Aside from your implementation for some reason delegating to operator-=, which is not. Same for operator+ which uses operator+=.

  6. Is there any reason you don't support subtracting a device_raw_ptr from another?

  7. I'm really puzzled why you make the only data-member protected instead of private.

\$\endgroup\$
  • \$\begingroup\$ Wouldn't the implicitly defined copy-assignment operator return a reference? \$\endgroup\$ – Yashas Apr 8 '19 at 5:05
  • \$\begingroup\$ I have incorporated the changes you suggested. Please check latest code \$\endgroup\$ – Yashas Apr 8 '19 at 7:43
2
\$\begingroup\$

Helper classes

There is no need to use static_cast<bool> for your comparisons. The relational operators are already bool values. (If they are not, that is a problem with the definition of the operator for the type T.)

The standard <utility> header provides definitions for operator!= (from operator==) and operator>, operator<=, and operator>= (from operator<). There is no need for you to define those four operators if you have the other two (equality and less-than).

Why do you have the relational_operators struct at all? It shouldn't be necessary.

Implementation

The default constructor for device_raw_ptr leaves the ptr member uninitialized. Typically a class like this would initialize ptr to nullptr, and you wouldn't need the constructor that takes a std::nullptr_t object.

The copy assignment operator should just be ptr = other.ptr, since that is the only thing in your class. The way you have it is nonstandard behavior. You construct a temporary, then pass it as a non-const reference to swap. This is not supported as part of the language, although some compilers (MSVC) support it as an extension. You're constructing a temporary, doing a swap, then destroying the temporary (a noop in this case). Similarly, the move assignment operator can be simplified to not use the temporary (ptr = other.ptr; other.reset();, or use three statements with an assignment to a local to avoid problems if you move assign an object to itself).

operator bool does not need a static_cast. Perhaps an explicit ptr != nullptr check, although a pointer will implicitly convert to a bool.

\$\endgroup\$
  • \$\begingroup\$ I thought using std::rel_ops is generally frowned upon. stackoverflow.com/questions/6225375/idiomatic-use-of-stdrel-ops suggests using boost:operators. I wrote my own version instead. \$\endgroup\$ – Yashas Apr 7 '19 at 17:07
  • \$\begingroup\$ The default constructor leaves ptr unitialized because that's how raw pointers behave (initialized with garbage?). But I am considering the option of initializing it to nullptr. The std::nullptr_t is a consequence of the aforementioned statement. I intended to have a constexpr constructor which sets ptr to nullptr. \$\endgroup\$ – Yashas Apr 7 '19 at 17:09
  • \$\begingroup\$ For the abuse of swap, I made started (codereview.stackexchange.com/questions/217019/…) using temporaries right after I posted the question. It was an attempt to reuse the constructors to perform move/copy but I think it was overkill for such a simple class. \$\endgroup\$ – Yashas Apr 7 '19 at 17:12
  • \$\begingroup\$ @Yashas std::rel_ops is there to avoid having duplicate definitions of those relational operators. If a class does not want some of them (as mentioned in your linked question), then it should define all of them, and = delete the ones it doesn't want to support. But its up to you to decide how to implement them. \$\endgroup\$ – 1201ProgramAlarm Apr 7 '19 at 17:14
  • \$\begingroup\$ @Yashas leaving raw pointer uninitialized is, generally, a bad idea. The cost of assigning a nullptr someplace where it isn't strictly necessary is outweighed by the predictability and consistent (mis)behavior the NULL value will give you. \$\endgroup\$ – 1201ProgramAlarm Apr 7 '19 at 17:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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