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

Cut backport of std::make_unique #1150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JoeLoser
Copy link
Contributor

@JoeLoser JoeLoser commented Jun 4, 2019

  • Previously, folly supported compilers which did not implement
    std::make_unique, so there exists a backport implementation.
  • Now that folly only supports C++14-compliant compilers, remove the
    backport.
  • Fix call sites which were relying on unqualified make_unique being
    found in the folly namespace to explicitly use std::make_unique.

- Previously, folly supported compilers which did not implement
  `std::make_unique`, so there exists a backport implementation.
- Now that folly only supports C++14-compliant compilers, remove the
  backport.
- Fix call sites which were relying on unqualified `make_unique` being
  found in the folly namespace to explicitly use `std::make_unique`.
@JoeLoser
Copy link
Contributor Author

Ping @yfeldblum

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.

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

@yfeldblum
Copy link
Contributor

Now that folly only supports C++14-compliant compilers, remove the backport.

Note that some parts of folly may be used with C++14 compilers but with C++11 standard libraries.

@JoeLoser
Copy link
Contributor Author

I wasn't aware of that bit, but are there any parts of folly in particular which need to be C++11-standard-library compatible? Based on the unqualified make_unique calls, it seems like nearly all of the call sites in folly namespace already use std::make_unique, so anyone depending on those headers is already using a standard library which has implemented std::make_unique. Do we have real code in the wild depending on this backport besides the two call sites I fixed in folly?

Does the backport live in folly as an easy way for downstream projects that are still using C++11-standard-library to get it, despite most of folly library already using std::make_unique?

@yfeldblum
Copy link
Contributor

Are there any parts of folly in particular which need to be C++11-standard-library compatible?

Well, compatible with the gcc49 implementation of the standard library in C++14 mode. This version has a partial but not a full implementation of the C++14 standard library spec.

Note that the gcc49 implementation of the standard library in C++14 mode ships with std::make_unique.

Beyond std::make_unique, for other names, there isn't really a system to know in advance.

@yfeldblum
Copy link
Contributor

Re this PR, there is code internally which still uses folly::make_unique, so that would need to be changed first.

@JoeLoser
Copy link
Contributor Author

I grepped through Thrift and Eden and there weren't any explicit uses of folly::make_unique (though there is a lot of using std::make_unique and using namespace std at the top of .cpp files to bring in make_unique into lookup for ADL.

Do you have any interest in fixing the internal code in a sweeping codemod or the like?

@yfeldblum
Copy link
Contributor

Yes if it's easy, no if it's hard. :)

I did just land one codemod internally, although more will be needed.

Based on a quick search, it looks like React Native is using folly::make_unique.

@JoeLoser
Copy link
Contributor Author

JoeLoser commented Oct 5, 2019

Here is a PR which replaces the usages of folly::make_unique with std::make_unique in the react-native project.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 22, 2019
Summary:
There is a mixed usage of `folly::make_unique` and `std::make_unique`. Soon, `folly::make_unique` may be removed (see [this PR](facebook/folly#1150)). Since `react-native` only supports C++14-compilers and later, switch to always using `std::make_unique`.

## Changelog

[Internal] [Removed] - Replace folly::make_unique with std::make_unique
Pull Request resolved: #26730

Test Plan:
Running the existing test suite. No change in behavior is expected.

Joshua Gross: buck install -r fb4a, make sure MP Home and forced teardown works okay on android

Reviewed By: shergin

Differential Revision: D18062400

Pulled By: JoshuaGross

fbshipit-source-id: 978ca794c7e972db872a8dcc57c31bdec7451481
@JoeLoser
Copy link
Contributor Author

ping @yfeldblum since facebook/react-native#26730 landed

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

3 participants