String in union, segfault

Question

This is basically a tagged union:

#include <string>
using std::string;

struct test{
    test():tag(INT),i(0){};
    test(const test&)=delete;
    test& operator=(const test&)=delete;
    enum {STRING,INT} tag;
    union {
        string s;
        int i;
    };
    test& operator=(string s){
        this->s=s;
        tag=STRING;
        return *this;
    }
    test& operator=(int i){
        this->i=i;
        tag=INT;
        return *this;
    }
    ~test(){
        if (tag==STRING)
            s.~string();
    }
};

int main(){
    test instance;
    instance="string";
    return 0;
}

It compiles but every time it crashes with a Segmentation fault. I'm just out of curiosity it's a pretty complete union class, provided a custom destructor, no move and no copy, then why will this crash? Must I use a string* in a union? If so, why?

Your `this->s=s` is not valid unless ``this->s` is already a valid string, which it may not be. You should use a placement `new`... but only if `tag==B`... — rodrigo, Nov 03 '16 at 07:23
@rodrigo, you mean when `tag != B`. Only Then will an assignment be UB. — StoryTeller - Unslander Monica, Nov 03 '16 at 07:28
Unfortunately, you cannot use non-POD types in a union. A fairly typical and simple workaround for this is to wrap the union in a struct, and move the non-POD instance to the struct. — Cherkesgiller, Nov 03 '16 at 07:31
@StoryTeller: I mean when `tag == B` you must use `operator=` but when `tag != B` you must use placement `new`. — rodrigo, Nov 03 '16 at 07:32
@rodrigo, well I agree with that. And I think the tags need better names (sigh) — StoryTeller - Unslander Monica, Nov 03 '16 at 07:33
Story Teller, thank you for information, but copy the code to IDE and set c++11 but still it gives an error "illegal union member" — Cherkesgiller, Nov 03 '16 at 07:39
@CherkesgillerTural, IDE aside, does your compiler support c++11? [Because gcc does, for instance](http://ideone.com/N5Dscx) — StoryTeller - Unslander Monica, Nov 03 '16 at 07:47
@CherkesgillerTural, well it compiles on VS as well http://rextester.com/live/JYIP92960 — StoryTeller - Unslander Monica, Nov 03 '16 at 08:06

Basile Starynkevitch · Accepted Answer · 2016-11-03T23:25:00.467

4

Read carefully about the rule of five in C++11

This is wrong code:

 /// wrong code
  test& operator=(string s){
      this->s=s;
      tag=STRING;
      return *this;
  }

because you are wrongly presuming that this->s is valid before assigning it. You should use a placement new which constructs it (in an uninitialized memory zone starting at &this->s):

  test& operator=(string ss){
      if (tag==STRING) { s=ss; return *this; }
      new((void*)&s) std::string(ss);
      tag=STRING;
      return *this;
  }

BTW, I guess that you also should explicitly define

test(const test&s);
test(const std::string&);
test& operator=(const test&);
test(test&&s);
test& operator=(test&&);

BTW you are obviously missing the constructor:

test(const std::string&str) {
  tag = STRING;
  new ((void*)(&s)) std::string(str);
}

edited Nov 03 '16 at 23:25

answered Nov 03 '16 at 07:45

Basile Starynkevitch

223,805
18
296
547

Is it safe to overwrite a it if it's previously a string? – YiFei Nov 03 '16 at 08:00
Yes, but what is the relation with your question or with my answer? My answer is precisely to emphasize that `s` i.e. `this->s` is *not* yet a valid string – Basile Starynkevitch Nov 03 '16 at 08:02
Yeah, I get your point, I'm just wondering if there's a if-statement needed in the `operator=`. – YiFei Nov 03 '16 at 08:05
This is inefficient - if `tag == STRING` you can just assign and return. No need to destroy and then construct again. – T.C. Nov 03 '16 at 19:45
Agreed. Will improve – Basile Starynkevitch Nov 03 '16 at 23:24

String in union, segfault

1 Answers1

Linked