Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Use empty base optimization with multiple inheritance #844

Merged
merged 2 commits into from
Nov 22, 2019

Conversation

droberts195
Copy link
Contributor

The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases). The investigation in #842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.

The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in elastic#842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.
Copy link
Contributor

@edsavage edsavage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@droberts195 droberts195 merged commit e0ff909 into elastic:master Nov 22, 2019
@droberts195 droberts195 deleted the optimize_empty_bases branch November 22, 2019 15:39
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Nov 22, 2019
The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in elastic#842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.

Backport of elastic#844
droberts195 added a commit that referenced this pull request Nov 22, 2019
The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in #842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.

Backport of #844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants