current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

The idea being you can have a simple type called maybe that wraps another type, which provides the ability to possibly not return a value from a function. I'm sure it's not perfect yet, but it's got the broad strokes I'm looking for. I'm mostly looking for corner cases I've missed and anything else I've overlooked.

#ifndef MAYBE_H_
#define MAYBE_H_

#include <exception>

namespace {              
    // implementation enabled for non-reference types
    template <class T>        
    struct maybe { 
        maybe()             : valid(false) {}       // default constructor

        maybe(const T& val) : valid(true)  {        // contructor from wrapped value
            new ((void*)storage) T(val);
        }

        maybe(const maybe &other) {                 // copy constructor
            valid = other.valid;
            if (valid) {
                new ((void*)storage) T(other.get_ref());
            }
        }

        maybe(maybe &&other) : maybe() {            // move constructor
            std::swap(valid,   other.valid);
            std::swap(storage, other.storage);
        }

        ~maybe() {                                  // destructor
            if (valid) {
                get_ref().~T(); 
            }
        }


        // assignment operator, uses copy-and-swap trick
        maybe& operator =(maybe other) {
            swap(*this, other);
            return *this;
        }


        // type-coercion operator to get back to wrapped value
        // calling without checking validity causes bad_exception
        operator T&() {                             
            if (!valid) {
                throw std::bad_exception();
            }
            return get_ref();
        }

        // for checking validity of container
        explicit operator bool() { return valid; }  
    private:
        bool valid;
        alignas(T) char storage[sizeof(T)];

        T& get_ref() const { return *static_cast<T*>((void*)storage); } 
    };


    // implementation enabled for reference types
    template <class T>        
    struct maybe<T&> {
        maybe()                   : valid(false)        value(nullptr)     {}
        maybe(T& val)             : valid(true),        value(&val)        {}
        maybe(const maybe &other) : valid(other.valid), value(other.value) {}

        operator T&() {
            if (!valid) {
                throw std::bad_exception();
            }
            return *value;
        }

        explicit operator bool() { return valid; }            
    private:
        bool valid;
        T*   value;
    };
} // namespace {

#endif//MAYBE_H_
share|improve this question
1  
Actually, appending improved code is no longer encouraged. Current policy is to either post a CW self-answer or post a new question. –  Jamal♦ Nov 3 '14 at 1:22

2 Answers 2

up vote 4 down vote

Let's go over it bit-by-bit:

  1. You should sprinkle around quite a lot of constexpr and noexcept-markers.
    If at least one instantiation of a template is potentially constexpr, marking the template thus is not an error. Not marking it means none is.
  2. union{T value;}; better shows the intent and is simpler than alignas(T) char storage[sizeof(T)];.
  3. Do not values with byte-for-byte unless they are trivial types. Doing so is simply wrong.
  4. The move-ctor should move, not swap. That might be far less expensive anyway.
  5. Your base variant should gain a specialization for trivially copyable types.
  6. Your base-variant should have a variadic-template ctor for constructing the value directly.
  7. Your references-variant does not need a separate valid-flag: *NULL is not valid, thus a null pointer serves adequately.
  8. Only include the minimal headers you need in your include-file.
  9. Do not use an unnamed namespace, or every translation-unit will have its own unique template (which is different from all the others).

As a side-note, boost::optional should serve for the case you describe.

share|improve this answer
    
All excellent thoughts, thank you. How does the union trick work? How would I specialize on trivial copyability? What do you mean about the variadic template-constructor? Thanks for the input! –  gct Nov 2 '14 at 23:41
    
Quite simply, because T is a member of a union, neither its ctor nor its dtor will be called. Because its a member of an anonymous union, it will actually be a member of the containing class. Nothing more to say about that. You specialize on trivial copyability by defining another specialization and using SFINAE to throw it out if not viable. And since C++11, you can have variadic templates, which means templates accepting an unlimited number of arguments. template<class... X> void f(X... x); is an example declaration. You might want to look at what boost does there. –  Deduplicator Nov 2 '14 at 23:48
up vote 2 down vote

If you have a construct by reference:

    maybe(const T& val) : valid(true)  {
        new ((void*)storage) T(val);
    }

Why not have a construct by move:

    maybe(T&& val) : valid(true)  {
        new ((void*)storage) T(std::forward<T>(val));
    }

You are swapping array's of char when you swap the storage.

    maybe(maybe &&other) : maybe() {            // move constructor
        std::swap(valid,   other.valid);
        std::swap(storage, other.storage);
    }

But what if the type T (stored in storage) is not swappable at the byte level. Very probable (that's why we have copy/move constructors). So when you do the swap here you should use T's swap.

    maybe(maybe &&other) : maybe() {
        std::swap(valid,   other.valid);

        using std::swap;
        swap(get_ref(),    other.get_ref());
    }

You have a copy assignment operator:

    // assignment operator, uses copy-and-swap trick
    maybe& operator =(maybe other) {
        swap(*this, other);
        return *this;
    }

May as well write a move assignment operator:

    // assignment operator, uses copy-and-swap trick
    maybe& operator=(maybe&& other) {

        using std::swap;
        swap(*this, other);

        return *this;
    }

OK. I see you were trying to treat references differently from normal types.

template <class T>        
struct maybe<T&> {

But I don't understand why you want to treat them differently.

share|improve this answer
    
You want to treat references differently from non-references, because you really don't want to swap the referred objects instead. –  Deduplicator Nov 3 '14 at 0:18
    
you have to treat reference specially from the base types because among other things, you can't construct a reference using placement new like you can with the others, so you kind of have to handle it differently. For copy assignment, I take my argument by value so I can use copy-and-swap, so it works as the move assignment too. Good call on the rvalue constructor though, is std::forward required in that context? –  gct Nov 3 '14 at 0:50
    
@gct: I do not think it is a good idea to have a special case for reference. Without the template specialization it would be an error to create a version for references so your argument is moot. What I need is an explanation of WHY you you think you need a reference version. –  Loki Astari Nov 3 '14 at 2:00
    
I need one because sometimes I have functions that would normally return a reference (ie the result of hash table lookup) but may not return a value (key not found). Another example would be returning the result of a matrix inverse. It's not practical to check for invertibility in the general case, so you have to just try and maybe fail. –  gct Nov 3 '14 at 2:28
    
@gct: For copy assignment,.... Still need a move assignment. –  Loki Astari Nov 3 '14 at 2:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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