C++11 class with union of string and shared_ptr

Question

(this is a bit similar to this question, and inspired by the C++11 FAQ on union but not exactly so...)

In the context of coding a Scheme-like interpreter in idiomatic C++11 Let's suppose I want a tagged union of a string, an int, and some closure. So I would probably code:

#include <string>
#include <new>
#include <memory>
#include <functional>
enum kind { nothing, string, integer, closure };

class Value {
 enum kind k;
 typedef std::string string_t;
  union {
    std::string str;
    int num;
    std::shared_ptr<std::function<Value(std::vector<Value>)>> clos;
  };
public:
 Value (const Value &v) 
 : k(none) {
   switch (v.k) {
   case none: break;
   case string: new(&str)string_t(v.str); break;
   case integer: num = v.num; break; 
   /// what about closure-s?
   }
   k = v.k;
 };
 Value& operator = (const Value&v) {
   switch (v.k) {
    case none: break;
    case string: new(&str)string_t(v.str); break;
    case integer: num = v.num; break; 
    /// what about closure-s?
   }
  k = v.k;
 }
 /// etc...
};

Now what about the closure case? For the copy constructor and the assignment operator, I am tempted to code:

 case closure: clos = v.clos; break;

But perhaps should I use a placement new on a shared_ptr?

I don't want to use Boost (or any non standard C++11 library) for that purpose.

Why are you hesitating to use placement `new` for the `shared_ptr` as for the `string`? I don't think that assignment of a `shared_ptr` to uninitialized memory is safe. It will potentially try to destruct the “managed object” of some garbage bits. Oh, and in the copy-assignment operator you should destroy the `string` before re-constructing it or it will leak the old buffer. — 5gon12eder, Feb 19 '15 at 17:44
This probably deserves to become an answer, not a comment.... — Basile Starynkevitch, Feb 19 '15 at 17:45
If you do not want to use Boost directly you should at least learn from there how it is done type safe way in `boost::variant` and use similar methods ie visitor pattern instead of switch etc. At least you will detect if visitor does not handle all variants at compile rather than runtime in your case. — Slava, Feb 19 '15 at 18:26
@Slava Given that the question is about C++11's `union`s, I don't think that `boost:variant` is a good source of inspiration because a large part of its purpose is to work around the restrictions that were in place for `union`s before C++11 relaxed them. — 5gon12eder, Feb 19 '15 at 19:18

score 4 · Accepted Answer · edited May 23 '17 at 10:28

I don't see any reason not to use placement new for the std::shared_ptr just as you did for the std::string. Simply assigning a value as in

clos = v.clos;

is not safe as it will call the copy-assignment operator of std::shared_ptr with a this pointer that potentially points to garbage memory. It might try to delete something that isn't there.

Likewise, in your copy-assignment operator, you should destroy the old object before you emplace the new one or the old value will leak.

using std::string;
using std::shared_ptr;
if (&v == this)
  return *this;  // self-assignment
switch (this->k)
  {
  case string:
    this->str.~string();
    break;
  case closure:
    this->clos.~shared_ptr();
    break;
  }
this->k = nothing;
// Now we are ready to take the new value.

The code will probably become easier to maintain if you use the copy & swap idiom. Since I don't see any obvious performance gains of not using it, I don't think it will cost you anything extra here.

C++11 class with union of string and shared_ptr

1 Answers1

Linked