My C++ gotcha of the day

Started by
5 comments, last by Juliean 7 years, 11 months ago

Here's conceptually-similar code: (part of code to render some 2D sprites on-screen)


struct Color
{
   uint8_t r = 255;
   uint8_t g = 255;
   uint8_t b = 255;
   uint8_t a = 255;
};

struct Vertex
{
   Color color;
   Other stuff;
};

union QuadCorners
{
    struct { Vertex topLeft, topRight, bottomRight, bottomLeft; };
    Vertex vertices[4]; //type-punning
};

And elsewhere in my code I save a copy of the current vertices for "settings".


struct Settings
{
    QuadCorners corners;
    Other stuff;
};

class Editor
{
public:
    void PlaceNewQuad(vectpr4reals position)
    {
        Quad *newQuad = CreateNewQuad(position);
        ApplySettingsToQuad(*newQuad);
    }
    
    void ApplySettingsToQuad(Quad &quad)
    {
        quad.corners = this->currentSettings.corners;
        quad.corners = this->currentSettings.corners;
    }

private:
    Settings settings;
};

Well, for some reason, my colors were all gibberish. :P

[rollup="Explanation"]Color's member variables aren't being initialized because its constructor isn't getting called because it's in a union, and the union doesn't know whether to treat the union as the four 'Vertex' members in the anonymous struct or as an array of 4 elements of type Vertex.

This is a minor 'gotcha' and one I should've caught quickly, but two things distracted me:

A) The obvious 'no-no' with unions is that you can't write to one member and read from the other - but since I'm doing that intentionally for type-punning, I wasn't worried, and was distracted by the 'obvious' gotcha, that I forgot that the members wouldn't be initialized.

B) I was also doing funky pointer stuff on the vertices elsewhere, so I assumed that must've been the location that was mucking up the color values. :) [/rollup]

I only realized what was going on when I stepped through the code with the debugger and saw that Editor::settings started off with bad values.

Advertisement

Huh, when its unclear where the issue is coming from, thats the worst :/

Had something along those lines for some time now. So in my runtime-type system I had a SFINEA-expression to check if a registered object has a reference-counter, or not:


template<typename ObjectType>
static auto AddRef(ObjectType* pObject, int) -> decltype(pObject->AddRef(), void())
{
    if(pObject)
        pObject->AddRef();
}

template<typename ObjectType>
static void AddRef(ObjectType* pObject, long)
{
}

For certain types under certain conditions, this simply was not working. Even though the object had a reference-counter, AddRef was called with the empty implementation, in certain parts of the code. When debugging explicitely, this behaviour somehow depended on which files where included, whether I forward-declared the type in question or not, and so on. There was no clear pattern to it, and I thought it was a compiler bug for the longest time.

So than the other day, I noticed that at a certain point I was doing this:


template<typename Type>
class Asset; // this is forward declared in another header

class MyObject
{
	// this is a fully declared class
};

 // exports dll-symbols for the type system for MyObject and Asset<MyObject>
EXPORT_ASSET_OBJECT(MyObject);

At this point, I was generating the export-symbols for the refCounter<Asset<MyObject>>, one of the types that were making difficulties. Turns out that because "Asset" is only forward declared here, it doesn't have an "AddRef"-method as far as the compiler can see it, so the empty SFINEA-expression is generated -.-

This explained why it depended on whether I included specific files, and/or was using the DLL symbol in the test project or not, and so on.

Thats really weird, I expected for SFINEA to eigther give some compilation error that the class was not defined at this point, but not just silently tread it as if it was an empty class the whole time.

I expected for SFINEA to eigther give some compilation error that the class was not defined at this point, but not just silently tread it as if it was an empty class the whole time.


Huh. Any idea what SFINAE stands for? It should be a clue as to why it's unreasonable to expect an error in that situation.

Stephen M. Webb
Professional Free Software Developer

Your code is invalid, but irrespective of the use of union (and your compiler is apparently broken because it does not notice). Your inline initialization of the color values in Color is equivalent to a default contructor, and members with constructors are not allowed in anonymous aggregates. That is, you are not allowed to write anything like struct { HasConstructor foo; };

Your "type punning" use of the union is actually legitimate because the standard (as of C++09 if I am not mistaken, but could as well be C++11) explicitly makes it well-defined as long as there is a common sequence. It is reasonable to assume that an array of identical objects which require no padding has a common sequence with several subsequent objects of the same type. Not sure if it's guaranteed (probably isn't), but the assumption is by all means reasonable.


This reminds me of a different topic "Are accessors evil?" from not long ago. You can rewrite your code without the troubles of unions like so:


struct QuadCorners2
{
    Vertex vertices[4];

    Vertex& topLeft() { return vertices[0]; }
    Vertex& topRight()  { return vertices[1]; }
    Vertex& bottomRight(){ return vertices[2]; }
    Vertex& bottomLeft() { return vertices[3]; }
};


The only downside is that now you have to write somevertex.topLeft() = blah; instead of somevertex.topLeft = blah;

However, on the positive side, you now have a plain normal struct which will default-initialize each element of its member array per §8.5. For arrays of PODs or arrays of types without default constructor, that will still do nothing, but Color has a default constructor, so everything is good.

(I left out const accessors in order not to bloat the code snippet up too much, you may want to add those)

So, in reply to "Are accessors evil?" one could say: "Accessors just saved my ass".

Huh. Any idea what SFINAE stands for? It should be a clue as to why it's unreasonable to expect an error in that situation.

Sure do, and it was an "Aha! That makes sense"-moment after I found out. Saying I expected an error was probably wrong, but I sure didn't think of this as even a possibility for this error.

Yea, I would've assumed the template generation would've been delayed until the full definition of the class became available - not treated as an empty class.

Your code is invalid, but irrespective of the use of union (and your compiler is apparently broken because it does not notice).

It's a psuedocode example, not my real code. What part did I mistype?

members with constructors are not allowed in anonymous aggregates. That is, you are not allowed to write anything like struct { HasConstructor foo; };

Bwah? Why not? I get in unions, it doesn't know which to call, but why not in anonymous structs?

I never knew that, thank you for the insight.

So, in reply to "Are accessors evil?" one could say: "Accessors just saved my ass".


I don't think accessory are evil - I use them frequently, where appropriate.

Since I was wanting the array type-punning for iteration, my "solution" to my above problem was adding it explicitly (and making the class a struct):


    auto begin()       { return &topLeft; }
    auto begin() const { return &topLeft; }
    auto   end()       { return ((&topLeft) + 4); /* one past the final element */ }
    auto   end() const { return ((&topLeft) + 4); /* one past the final element */ }
    auto  size() const { return (end() - begin());  }
          Type &operator[](size_t i)       { return *(this->begin() + i); }
    const Type &operator[](size_t i) const { return *(this->begin() + i); }

Huh. Any idea what SFINAE stands for? It should be a clue as to why it's unreasonable to expect an error in that situation.

Sure do, and it was an "Aha! That makes sense"-moment after I found out. Saying I expected an error was probably wrong, but I sure didn't think of this as even a possibility for this error.

I'd go as far as saying that's yet another good example of why macros are bad practice in C++ code.

Stephen M. Webb
Professional Free Software Developer

Huh? i dont see how the macro is related to this in this case, I generally do tend to avoid them but in this case it is just a shorthand for "extern ACCLIMATE_API template class XXX", and I do know what it stands for so whether seeing the macro or the written-out version doesnt seem to make a big difference to me.

This topic is closed to new replies.

Advertisement