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

Improvements and fixes for C++ nullable implementation and reduce dynamic allocation #174

Merged
merged 6 commits into from
Jun 9, 2016

Conversation

glenfe
Copy link
Contributor

@glenfe glenfe commented Jun 6, 2016

Covers points 1, 2, 3, 4, and 6 that I raised in issue #167 as well as reduce two additional dynamic allocations.

@msftclas
Copy link

msftclas commented Jun 6, 2016

Hi @glenfe, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@glenfe
Copy link
Contributor Author

glenfe commented Jun 6, 2016

The space saving is actually more than just 1 byte. That is, the empty base optimization there actually saves alignof(value_type) bytes in the case of the first specialization, and saves alignof(pointer) bytes in the case of the second specialization. e.g. This could be up to 8 bytes on 64-bit, or 4 bytes on 32-bit for certain nullable<T> types.

@@ -117,7 +118,7 @@ class nullable<T, Allocator, true>

explicit
nullable(const allocator_type& alloc)
: _alloc(alloc),
: allocator_type(alloc),
Copy link
Member

@chwarr chwarr Jun 7, 2016

Choose a reason for hiding this comment

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

I think it would be clearer if we used Allocator in these ctors, as that's the name of the base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@chwarr
Copy link
Member

chwarr commented Jun 7, 2016

Left some comments. Be sure to look at the comments on outdated diffs, because GitHub doesn't understand that reviewing individual commits is a thing. :-(

@glenfe
Copy link
Contributor Author

glenfe commented Jun 7, 2016

The rationale for !!pointer over static_cast<bool>(pointer) is, of course, that MSVC and its diagnostics are, as ever, defective. (You should later consider disabling that warning like you do so many others in warning.h)

@jasonzio
Copy link

jasonzio commented Jun 7, 2016

I'm not a big fan of disabling warnings. If you chose to do so, please wrap it tightly around the offending line of code; don't do it globally for the entire file or, worse yet, the entire project. Warnings really do help catch problems; a tightly-wrapped deviation (disable, then the offending line of code, then re-enable) shows the warning was investigated, determined to be benign, and silenced.

@glenfe
Copy link
Contributor Author

glenfe commented Jun 7, 2016

I'm a big fan of disabling stupid warnings, especially globally. It shows a sensible intolerance to a defective C++ implementation.

@chwarr
Copy link
Member

chwarr commented Jun 8, 2016

!!pointer seems like a good compromise.

@chwarr
Copy link
Member

chwarr commented Jun 8, 2016

Confirmed size reduction of bond::nullable<std::string> a pointer size (8 bytes [56 to 48] with Visual Studio 14 2015 Win64 and 4 bytes [36 to 32] with Visual Studio 14 2015). 🎉

I'm going to run some x86 builds myself, as these aren't part of the CI builds.

@chwarr chwarr merged commit 744a982 into microsoft:master Jun 9, 2016
chwarr added a commit that referenced this pull request Jun 9, 2016
Closes #174, "Improvements and fixes for C++ nullable implementation and
reduce dynamic allocation"
@chwarr
Copy link
Member

chwarr commented Aug 23, 2016

Bond version 4.3.0 is now live on NuGet.org. These changes are included in that version. You'll also need to pull down the latest C++ source code as well.

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

Successfully merging this pull request may close these issues.

4 participants