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.

Sign up for GitHub

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

Jump to bottom

Cut noexcept calculations in dynamic-inl.h #1034

Closed
wants to merge 1 commit into from
Closed

Cut noexcept calculations in dynamic-inl.h #1034

wants to merge 1 commit into from

Conversation

JoeLoser
Copy link
Contributor

Summary:

  • GCC 4.9 previously required marking TypeError special member
    functions with conditional noexcept.
  • GCC 5.1 does not have this issue. So, remove the conditional noexcept
    as they always evalaute to true in the existing code.

yfeldblum
yfeldblum requested changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@yfeldblum yfeldblum left a comment

Choose a reason for hiding this comment

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

Must also do .cpp.

@JoeLoser JoeLoser force-pushed the dynamic_noexcept_calculations branch from 9b8d066 to 3475baa Compare February 27, 2019 06:01
@JoeLoser
Copy link
Contributor Author

Thanks, just fixed the corresponding .cpp file as well. Any particular reason all five special member functions aren't just =defaulted in dynamic-inl.h?

@yfeldblum
Copy link
Contributor

There was a reason which appears no longer to be relevant. This was to outline these functions to shrink the code size of throw TypeError(...); statements.

We are now using throw_exception<TypeError>(...); statements which already takes care of shrinking the code size, so the reason is no longer applicable.

So there should be no problem with declaring all of these as defaulted within the class body, which would automatically take care of finding the proper noexcept specification.

@JoeLoser JoeLoser force-pushed the dynamic_noexcept_calculations branch from 3475baa to 54f7296 Compare February 27, 2019 15:02
@JoeLoser
Copy link
Contributor Author

Thanks for the explanation -- makes sense. I went ahead and just defaulted them in dynamic-inl.h.

yfeldblum
yfeldblum reviewed Feb 27, 2019
View reviewed changes
folly/dynamic-inl.h Outdated Show resolved Hide resolved
@JoeLoser
Cut noexcept calculations in dynamic-inl.h
9698eef 
Summary:
- GCC 4.9 previously required marking `TypeError` special member
  functions with conditional noexcept.
- GCC 5.1 does not have this issue, so defaulting them is sufficient. In
  fact, we can even let the compiler automatically generate these special
  member functions.
@JoeLoser JoeLoser force-pushed the dynamic_noexcept_calculations branch from 54f7296 to 9698eef Compare February 28, 2019 02:28
facebook-github-bot
facebook-github-bot reviewed Feb 28, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yfeldblum
Copy link
Contributor

I am internally rewriting the commit message on this one before landing to reflect that this is now about de-outlining the special member functions.

@JoeLoser
Copy link
Contributor Author

JoeLoser commented Mar 1, 2019

Nice reword of the commit. Thanks for that and helping it land!

sandraiser pushed a commit to sandraiser/folly that referenced this pull request Mar 4, 2019
@JoeLoser @sandraiser
Inline TypeError special member functions (facebook#1034)
e2ad640 
Summary:
The `TypeError` special member functions - copy and move constructors and assignment operators - were outlined to reduce inline code size of `throw TypeError(...);` statements. This is no longer necessary, since these sites have been replaced by `folly::throw_exception<TypeError>(...);` statements, which have minimal inline code size. As a benefit, the special member functions may all now be declared as defaulted within the class body, and the conditional `noexcept` calculations are no longer required to be written - the compiler will do them. (Note: as of the libstdc++ which ships with GCC 5, library exception special member functions are `noexcept`; previously, they are not.)

Pull Request resolved: facebook#1034

Reviewed By: nbronson

Differential Revision: D14255166

Pulled By: yfeldblum

fbshipit-source-id: 2f795a2b937fee58f243a9d374fc01829c674fe6
@JoeLoser JoeLoser deleted the dynamic_noexcept_calculations branch April 2, 2019 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yfeldblum yfeldblum yfeldblum requested changes

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JoeLoser @yfeldblum @facebook-github-bot