Skip to content

Conversation

@matthargett
Copy link

Summary:
These are a set of minor changes to allow building and linking on older clang and toolchains that use Dinkumware headers. I adjusted and tested only the subset of folly used by React Native's CxxReact, Fabric, and TurboModules C++ sources.

Test plan:
I ran the build on my Ubuntu 18.x using both clang 3.9, clang 6.x, and GCC 8.x. I couldn't find a way to build and run the unit tests in cmake, so I spot checked several unit tests with a manual CLI build:
g++-8 -I ../vcpkg/installed/x64-linux/include/ -L../vcpkg/installed/x64-linux/lib -I. -I_build folly/test/DynamicConverterTest.cpp folly/test/common/TestMain.cpp -lgtest -lpthread -lglog -lgflags -ldouble-conversion -lssl -lboost_filesystem -lboost_program_options -lboost_regex -lboost_system -ldl -lstdc++ -liberty _build/libfolly.a ../vcpkg/installed/x64-linux/lib/libdouble-conversion.a

@matthargett
Copy link
Author

@mhorowitz this is the folly side of facebook/react-native#21764

@matthargett
Copy link
Author

Looks like the build is failing for unrelated reasons (can't persist the ccache after the build finished successfully).

@yfeldblum
Copy link
Contributor

To give some perspective, overall, we are not planning to support older versions of compilers. This includes clang 3.9 and msvc 2015.

As one particular, clang 3.9 has no support for C++17, so that compiler will definitely not work once we decide to jump to C++17.

We also want to avoid all the extra complexity, preprocessor conditionals, restrictions on coding patterns, etc., that arise from supporting older compilers.

@matthargett matthargett changed the title Improve compatibility with older clang and Dinkumware C++ headers Improve compatibility with Dinkumware C++ headers Feb 25, 2019
@matthargett
Copy link
Author

@yfeldblum sorry, my title was misleading. This is with clang 6 and 7, but using Dinkumware headers.

I was able to bump the Dinkumware headers and eliminate a bunch of the __has_include noise. Take another look and let me know what you think.

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.

Can you clarify which versions of cpplib you are intending compatibility with?

@yfeldblum
Copy link
Contributor

Note that when PRs are imported into the internal code review tool or landed into the repo, the PR commits are squashed into a single commit.

#include <folly/portability/Windows.h>

#if __has_include(<pthread_np.h>)
#include <pthread_np.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Orvid Do we want this include here or in the portability header?

@matthargett
Copy link
Author

Looks like the build failure is an unrelated regression caused by a different commit?

/home/folly/folly/small_vector.h:739:32: error: 'malloc_usable_size' was not declared in this scope
       return malloc_usable_size(u.pdata_.heap_) / sizeof(value_type);```

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

Note that I will internally reorder the glog includes to match our style.

@matthargett
Copy link
Author

any issues landing it in phabricator? let me know if there's anything else I can do to get it merged.

// are found in stdlib.h.
#if __has_include(<malloc.h>)
// are found in stdlib.h and it errors when including malloc.h.
#if __has_include(<malloc.h>) && (!defined(__STDC__) || !__STDC__)
Copy link
Contributor

Choose a reason for hiding this comment

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

This (or the change to the portability malloc below) is causing malloc.h to no longer get included in our internal builds :(

If these two changes are dropped, the rest should be safe to land :)

@matthargett
Copy link
Author

A quick note on the uplift of folly and CxxReact before I close this. when I was doing this work, it replaced 50+ megabytes of mono AOT binaries with around 50kb of extremely fast C++ in the React Native system software apps I was working on. besides needing to have as much free RAM and compute as possible for the games that ran at the same time as the apps, it also meant we would've been able to increase the size of our in-memory Apollo GraphQL cache to keep the system running even more fluidly.

I no longer work for the platform holder that has the wholly unique marriage of clang trunk and ancient Dinkumware headers, so I'll close this and someone who's still employed there can resubmit if they see fit. if, by some bizarre happenstance there is another platform that has FreeBSD, clang with c++2a features, with Dinkumware headers, they can also carry it forward.

thanks for working through the review with me, sorry I left it dangling this long.

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.

4 participants