Cxx 17

[seelabs]

This PR is build on 1.3.0-b2 and proposes that we enable C++-17 support. I'm assigning @HowardHinnant and @nbougalis as reviewers: Howard for the code changes and Nik to make the broader decision if we want to approve turning on c++ 17.

Just the commits after Set version to 1.3.0-b2 need to be reviewed.

[ripplelabs-jenkins]

Jenkins Build Summary

Built from this commit

Built at 20190805 - 17:52:47

Test Results

Build Type	Log	Result	Status
msvc.Debug	logfile	1178 cases, 0 failed, t: 582s	PASS ✅
gcc.Release -Dassert=ON, MANUAL_TESTS=true	logfile	915 cases, 0 failed, t: 5m0s	PASS ✅
msvc.Debug, NINJA_BUILD=true	logfile	1178 cases, 0 failed, t: 612s	PASS ✅
gcc.Debug -Dcoverage=ON, TARGET=coverage_report, SKIP_TESTS=true	logfile	1178 cases, 0 failed, t: 18m29s	PASS ✅
docs, TARGET=docs	logfile	1 cases, 0 failed, t: 0m1s	PASS ✅
clang.Debug	logfile	no test results, t: 6s [BAD EXIT]	FAIL 🔴
clang.Debug -Dunity=OFF	logfile	no test results, t: 6s [BAD EXIT]	FAIL 🔴
msvc.Debug -Dunity=OFF	logfile	1177 cases, 0 failed, t: 1138s	PASS ✅
gcc.Debug	logfile	1178 cases, 0 failed, t: 3m1s	PASS ✅
msvc.Release	logfile	1178 cases, 0 failed, t: 415s	PASS ✅
gcc.Debug -Dunity=OFF	logfile	1177 cases, 0 failed, t: 12m23s	PASS ✅
clang.Release -Dassert=ON	logfile	no test results, t: 9s [BAD EXIT]	FAIL 🔴
gcc.Release -Dassert=ON	logfile	1178 cases, 0 failed, t: 5m3s	PASS ✅
gcc.Debug -Dstatic=OFF	logfile	1178 cases, 0 failed, t: 2m58s	PASS ✅
gcc.Debug -Dstatic=OFF -DBUILD_SHARED_LIBS=ON	logfile	1178 cases, 0 failed, t: 3m0s	PASS ✅
gcc.Debug, NINJA_BUILD=true	logfile	1178 cases, 0 failed, t: 2m54s	PASS ✅
clang.Debug -Dunity=OFF -Dsan=address, PARALLEL_TESTS=false, DEBUGGER=false	logfile	no test results, t: 6s [BAD EXIT]	FAIL 🔴
clang.Debug -Dunity=OFF -Dsan=undefined, PARALLEL_TESTS=false	logfile	no test results, t: 5s [BAD EXIT]	FAIL 🔴

[scottschurr]

FWIW, regarding the clang build failures, I tried a local clang build on my Mac OS machine and got this diagnostic:

In file included from /Users/scottschurr/Development/rippled/src/ripple/core/impl/SNTPClock.cpp:22:
/Users/scottschurr/Development/rippled/src/ripple/basics/random.h:54:10: error: 
      no template named 'is_invocable_r' in namespace 'std'; did you mean
      '__invokable_r'?
    std::is_invocable_r<Result, Engine>;
    ~~~~~^~~~~~~~~~~~~~
         __invokable_r
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4336:8: note: 
      '__invokable_r' declared here
struct __invokable_r
       ^

[seelabs]

@scottschurr The problem is the version of xcode you're using ships with the equivalent to clang 5; Support for is_invocable_r didn't happen until clang 6. Unforuntately, this requires our mac users to be on the xcode 10, which only came out last September.

The options I see are:

1. Require xcode 10;
2. Code up a workaround for is_invocable_r for xcode 9
3. Don't upgrade to c++-17 yet.

Right now, I'm leaning to (3). But I'll leave this PR open for a little while longer.

[HowardHinnant]

I can confirm that things compile on Xcode 10.

My opinion is that neither requiring Xcode 10, nor patching is_invocable_r for Xcode 9 are big enough deals to impact the decision to move to C++17. The decision should be made on other factors such as the cost to those clients who are compiling with gcc (our only officially supported platform), and what benefits C++17 will bring us (CTAD seems like a very useful language feature to me).

I also believe that we probably need to upgrade beyond gcc-5.2 prior to moving to C++17.

[HowardHinnant]

This passes all tests on macOS, using Xcode 10. The code changes look good to me.

[scottschurr]

FWIW, if all that is required is a relatively simple work around for Xcode 9 I'd be in favor of pushing ahead. There are some specific capabilities if C++17 that I'd like to use in the code base. I can think of inline variables and if constexpr specifically.

[HowardHinnant]

if constexpr isn't supported by gcc-5.2. We need at least gcc-7 for that. I haven't checked inline variables. I agree these would both be good to have in our toolbox.

[HowardHinnant]

Here is what src/ripple/beast/cxx17/type_traits.h could look like to patch Xcode 9:

#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 6000

namespace std
{

template <class _Ret, class _Fn, class ..._Args>
struct _LIBCPP_TEMPLATE_VIS is_invocable_r
    : integral_constant<bool, __invokable_r<_Ret, _Fn, _Args...>::value> {};

}  // namespace std

#endif

[seelabs]

Forced push with Howard's patch for xcode 9

[scottschurr]

With @HowardHinnant's patch everything builds, unit tests pass, and server syncs and shuts down cleanly on my Mac using Xcode 9! Nice work!

[seelabs]

The problem with the clang jenkins builds is clang is picking up the standard library from gcc 6 (even though gcc 7 is installed) and gcc 6 does not have string_view.

@mellery451 and I brainstormed about this and see two options:

1. We could update the jenkins clang to clang 7. This does pick up the correct stdlib. The downside to that is we no longer have a CI test for clang 5 at all (xcode 9 still uses clang 5, so I don't want to lose that test).

2. Upgrade the jenkins to clang 7, and add a mac tests to travis. Mike says travis has good support for mac, so as much as I dislike travis, this may be a good solution, at least until we stop supporting xcode 9.

We think (2) is the best option.

[nbougalis]

I'm supportive of merging this, but let's make sure that it includes the necessary CI changes to ensure ongoing coverage.

[JoeLoser]

Is there any consensus on how to move forward with this? I'd love to see this land.

[sublimator]

Winter is coming

…

[seelabs]

@JoeLoser The plan is to land this early into the 1.4 release. I don't want to put it into the 1.3 release, which is too far along to bump the C++ version.