Use two full size members instead of using a static array as a union #8620
Conversation
ronie
added
Type: Fix
|
thanx! will test it and let you know. @tamland / @jimfcarroll for review |
|
Tested on arch with gcc 5.3 (stdc 6.0.21), isengard 15.2 and works for me :) |
|
Oddly enough it does. @klusark have you tried using a regular union here instead of the array? |
|
@tamland that was my first thought as well, but c++ does not allow unions of types with constructors. |
|
confirmed on ubuntu 15.10 / gcc 5.2.1 as well. |
|
@klusark Should be possible http://en.cppreference.com/w/cpp/language/union (see 'Explanation') as long as you implement all special members. I'm guessing that's the root cause here. It relies on defaults so same a unions, the destructor of T1 or T2 will never be called for one thing. (Just use the two full size members though. It's too hard to implement this right.) |
| @@ -31,58 +31,52 @@ namespace XBMCAddon | |||
| public: | |||
| private: | |||
| WhichAlternative pos; | |||
| unsigned char m_data[sizeof(T1) > sizeof(T2) ? sizeof(T1) : sizeof(T2)]; | |||
| T1 d1; | |||
| T2 d2; | |||
|
|
|||
| public: | |||
| Alternative() : pos(none) {} | |||
| Alternative(const Alternative& o) | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This really shouldn't change anything, but the current implementation fails to work correctly when using the new C++11 ABI in gcc 5.1 and up. I'm hoping someone will be able to find the root cause, but this is better than nothing.
|
I spent around an hour trying to get it working with a union. I got it to compile, but it kept segfaulting in random places. Probably not worth the effort. |
|
jenkins build this please |
Use two full size members instead of using a static array as a union
|
@klusark can you create a PR for the Jarvis branch as well? |
|
@tamland will do |
|
I don't understand why it wasn't working. If anything it should have been a memory leak since any "Alternative" that has a destructor wouldn't have been called. But other than that I don't understand what was happening. |
|
@jimfcarroll My guess is that it has something to do with using placement new on approximately the same stack address repeatedly along with some undefined behavior in the standard library. |
|
@jimfcarroll It didn't implement copy assignment operator, so the default one is generated making shallow copies. |
This really shouldn't change anything, but the current implementation
fails to work correctly when using the new C++11 ABI in gcc 5.1 and up.
I'm hoping someone will be able to find the root cause, but this is better
than nothing.
Bugs to reference:
http://trac.kodi.tv/ticket/16414
http://trac.kodi.tv/ticket/16421