Skip to content

Conversation

@arookas
Copy link
Contributor

@arookas arookas commented Jun 2, 2019

The MSVC EBO problem (#8) has already been fixed for totally_ordered and a large number of other operators; however, the fix is missing from basic arithmetic operators and other equality operators. I noticed this when making a matrix type using multipliable and requiring strict size and alignment properties for use in OpenGL UBOs.

This adds the TAO_OPERATORS_BROKEN_EBO to the remaining classes, particularly the BASIC_OP ones, allowing other operators to be used without increasing the class size on MSVC. A couple of the additions may be redundant in practice.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ba883d7 on arookas:master into 122743a on taocpp:master.

@d-frey
Copy link
Member

d-frey commented Jun 3, 2019

Thanks!

@d-frey
Copy link
Member

d-frey commented Jun 3, 2019

I am trying to understand why those additional __declspec(empty_bases) help. According to some documentation I found (https://devblogs.microsoft.com/cppblog/optimizing-the-layout-of-empty-base-classes-in-vs2015-update-2-3/), it appears that only derived classes with multiple base classes need to be marked, but your patch marks the basic (empty) classes that have no base classes. Can you explain how the PR helps and point me to some documentation from MS, please?

@arookas
Copy link
Contributor Author

arookas commented Jun 4, 2019

I'm trying to understand it, as well. Couldn't find any specific documentation on empty_bases but doing some hard tests finds some interesting undocumented behavior. Compiling on the same environment as before:

class empty_base_1 {};
class empty_base_2 {};

class derived : empty_base_1, empty_base_2 {
  int i;
};

static_assert(sizeof(derived) == sizeof(int));

This fails with a C2607; however, any combination of __declspec(empty_bases) on any of these three classes passes successfully. For instance:

class __declspec(empty_bases) empty_base_1 {};
class __declspec(empty_bases) empty_base_2 {};
:
class derived : empty_base_1, empty_base_2 {
:

or:

class empty_base_1 {};
class empty_base_2 {};
:
class __declspec(empty_bases) derived : empty_base_1, empty_base_2 {
:

or even more strangely:

class __declspec(empty_bases) empty_base_1 {};
class empty_base_2 {};
:
class derived : empty_base_1, empty_base_2 {
:

Although the blog documentation states it applies to a class which has multiple inheritance, specifying it on the non-inheriting base class appears to make it affect its derived classes automatically, regardless of such a class's own empty_bases.

On the other hand, this could mean the proper way to solve this is to apply __declspec(empty_bases) to my own types using this library when I happen to need to inherit more than one. It doesn't bother me which so I'll leave it up to your discretion.

@d-frey
Copy link
Member

d-frey commented Jun 4, 2019

Thinking about it, it does make sense: Without any additional information, MS remains ABI-compatible. Once a class (any class) in the hierarchy is marked as new-style-EBO-enabled, they can switch to the newer ABI. This means I'll accept the PR as it will then enable the new EBO in more cases even when users don't explicitly add __declspec(empty_bases) to their classes. Thanks again!

@d-frey d-frey merged commit 79f967c into taocpp:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants