You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by Typz <gi...@git.apache.org> on 2017/12/22 15:16:33 UTC
[GitHub] thrift pull request #1448: [WIP] Support compilation without Boost
GitHub user Typz opened a pull request:
https://github.com/apache/thrift/pull/1448
[WIP] Support compilation without Boost
The goal of this series of patches is to support compiling Thrift library without boost: in C++11, most of the things are already integrated, and it is often not practical to integrate boost in embedded projects.
These patch use C++11 alternatives when possible, and default to Boost otherwise.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Typz/thrift boost-removal
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1448.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1448
----
commit 4b8688b0fab4624fed692e83f884307b32a463d1
Author: Francois Ferrand <th...@...>
Date: 2017-12-11T17:18:51Z
Replace boost::noncopyable with a custom implementation
commit 657ba7028a32390a57ed34a66d51963daa50ee56
Author: Francois Ferrand <th...@...>
Date: 2017-12-11T17:33:47Z
Replace boost::scoped_array with std::unique_ptr
Also replace boost::shared_array, since only scoped_array was needed.
commit c0b1bb1bd3eb623b3bd0145bec116d1b64b6aa68
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T09:10:50Z
Remove unused boost header inclusion
commit de1627987daebb5286998bfc13490be0af4e58a1
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T09:28:26Z
Replace boost::atomic with std::atomic
commit 4c6300a6d381fef36f6a139470c3f094759bb287
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T09:49:57Z
Replace boost::unique_ptr with std::unique_ptr
commit bb1c3e0cd9988efc3f1ede825a24041ea9574f67
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T10:22:14Z
Replace boost::tokenizer with std::getline
commit 939fbe89015bebf2e1223742ca217d1fbb1d0a90
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T11:03:28Z
Replace BOOST_STATIC_ASSERT with static_assert
commit 187487f8eab8b95b921d7ee6030f439ca4182214
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T14:08:06Z
Use fpclassify and signbit from std lib
commit 330d9f812d3a5157349d70ab322dad2715bbc72c
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T17:10:41Z
Use std::codecvt_utf8_utf16 for utf16 to utf8 conversion
commit e760eb4231710334b506df353918fbba73e5fe83
Author: Francois Ferrand <th...@...>
Date: 2017-12-12T17:32:19Z
Replace boost::numeric_cast with std::numeric_limits
commit a13452891a4826e255053441a137796222ab9c75
Author: Francois Ferrand <th...@...>
Date: 2017-12-14T10:52:16Z
Use ostringstream instead of boost::format
commit 8fb0c8461ddc974b2a7dc310e79f4501fc30341b
Author: Francois Ferrand <th...@...>
Date: 2017-12-14T11:32:03Z
Replace BOOST_SCOPE_EXIT with unique_ptr and custom deleter
commit cfad8e6165d7c18de1f52ea9979a8bc9d1e3c13a
Author: Francois Ferrand <th...@...>
Date: 2017-12-15T12:46:18Z
Remove use of boost::istarts_with and boost::iends_with
----
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1448
I tried to use this work and did a little bit of cmake work to straighten out options. What I found is that there is a boost dependency in the safe_numeric_cast in TTransportException.hpp and there's another boost dependency in TProtocol.h (for endian detection) that needs to be resolved.
What I would suggest is that you try building on Windows without boost, disabling build of tests and tutorials, so that all you build is the compiler and the C++ library.
You can see what I did in my referenced pull request in my fork.
---
[GitHub] thrift pull request #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1448#discussion_r161512237
--- Diff: appveyor.yml ---
@@ -45,7 +45,7 @@ environment:
- PROFILE: MSVC2015
PLATFORM: x64
CONFIGURATION: Release
- BOOST_VERSION: 1.64.0
--- End diff --
Why?
---
[GitHub] thrift pull request #1448: [WIP] Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1448#discussion_r159267705
--- Diff: lib/cpp/src/thrift/processor/TMultiplexedProcessor.h ---
@@ -165,10 +166,11 @@ class TMultiplexedProcessor : public TProcessor {
}
// Extract the service name
- boost::tokenizer<boost::char_separator<char> > tok(name, boost::char_separator<char>(":"));
-
std::vector<std::string> tokens;
- std::copy(tok.begin(), tok.end(), std::back_inserter(tokens));
+ std::istringstream tokenStream(name);
+ for (std::string token; std::getline(tokenStream, token, ':');) {
--- End diff --
std::getline is available also in pre-C++11 (though C++11 adds some overloads): http://en.cppreference.com/w/cpp/string/basic_string/getline
I would be OK with C++11, but this may not be applicable to all cases... Maybe a safer approach would be first allow use without boost (relying on C++11: i.e. this patch :-) ), then add C++11 requirement if this is acceptable.
---
[GitHub] thrift pull request #1448: Thrift-4441: Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1448#discussion_r161516311
--- Diff: appveyor.yml ---
@@ -45,7 +45,7 @@ environment:
- PROFILE: MSVC2015
PLATFORM: x64
CONFIGURATION: Release
- BOOST_VERSION: 1.64.0
--- End diff --
VS2015 image does not contain Boost 1.64 : it only contains versions up to 1.63.
1.64 and 1.65.1 are included only in VS2017 image. See https://www.appveyor.com/docs/build-environment/#boost for full details
This is not an issue introduced by this bug, but my patch highlights the problem: currently, this build expect Boost 1.64, which is not present, thus the C++ lib is not built.
With my patches, the C++ lib can be built (even without Boost), but the tests still require Boost: so building in this case would require an extra flag to disable tests.
Since the goal is (I think...) to actually build with a recent version of Boost, I downgraded the version the most recent installed version in the image.
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1448
@Typz the example for this conversion on cppreference is different than your code. Try that?
http://en.cppreference.com/w/cpp/locale/wstring_convert/to_bytes
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1448
Would it be possible for you to squash this to make the merge easier?
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on the issue:
https://github.com/apache/thrift/pull/1448
@jeking3 : any suggestion?
---
[GitHub] thrift pull request #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1448#discussion_r173620082
--- Diff: build/cmake/DefineOptions.cmake ---
@@ -91,11 +91,26 @@ if(WITH_CPP)
option(WITH_STDTHREADS "Build with C++ std::thread support" OFF)
CMAKE_DEPENDENT_OPTION(WITH_BOOSTTHREADS "Build with Boost threads support" OFF
"NOT WITH_STDTHREADS;Boost_FOUND" OFF)
+
+ set(WITH_CPP_SUPPORT OFF)
--- End diff --
I think we would be better off testing for C++11 features and ensuring that if we find what we need, we can enable certain things. For example if we ask cmake to check for support of cxx_defaulted_functions then we can enable the boost-less noncopyable. One can typically check for cxx_nullptr in order to determine if there is smart pointer support, or write a custom check for std::shared_ptr detection. Relying on checking compiler versions is error-prone and not as portable.
This is a good start to optionally eliminating boost from the C++ runtime library. I can work on the feature checks as an extension to this work.
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on the issue:
https://github.com/apache/thrift/pull/1448
Indeed, it is complete now: the lib can be built without Boost, both through autoconf or cmake.
Tests (based on boost test) and tutorial still require boost, though.
---
[GitHub] thrift pull request #1448: [WIP] Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1448#discussion_r158557467
--- Diff: lib/cpp/src/thrift/stdcxx.h ---
@@ -102,23 +108,71 @@ namespace apache { namespace thrift { namespace stdcxx {
using ::boost::enable_shared_from_this;
using ::boost::make_shared;
using ::boost::scoped_ptr;
+ using ::boost::scoped_array;
using ::boost::shared_ptr;
using ::boost::static_pointer_cast;
using ::boost::weak_ptr;
+#if (BOOST_VERSION >= 105700)
+ using ::boost::movelib::unique_ptr;
+#else
+ using ::boost::interprocess::unique_ptr;
+#endif
+
#else
using ::std::const_pointer_cast;
using ::std::dynamic_pointer_cast;
using ::std::enable_shared_from_this;
using ::std::make_shared;
template <typename T> using scoped_ptr = std::unique_ptr<T>; // compiler must support template aliasing
+ template <typename T> using scoped_array = std::unique_ptr<T[]>; // compiler must support template aliasing
using ::std::shared_ptr;
using ::std::static_pointer_cast;
using ::std::weak_ptr;
+ using ::std::unique_ptr;
+
+#endif
+
+}}} // apache::thrift::stdcxx
+
+///////////////////////////////////////////////////////////////////
+//
+// Atomic
+//
+///////////////////////////////////////////////////////////////////
+
+#if __cplusplus < 201103L
--- End diff --
This is always true on all Microsoft compilers (2010 - 2017). It isn't the best thing to be conditional on and be portable, unfortunately.
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on the issue:
https://github.com/apache/thrift/pull/1448
@jeking3 : This exemple refers to conversions from 'wide' strings to UTF8 or UTF16. I used this exemple: http://en.cppreference.com/w/cpp/locale/codecvt_utf8_utf16 which shows conversion from UTF16 to UTF8.
According to the name of the function, that should be what we expect; but maybe i was indeed mislead by the name, and it should be simply 'wide' to UTF8 ?
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on the issue:
https://github.com/apache/thrift/pull/1448
Squashed everything in one big patch.
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by Typz <gi...@git.apache.org>.
Github user Typz commented on the issue:
https://github.com/apache/thrift/pull/1448
I have an issue with UBSan build: it keeps failing in codecvt, which may be due either to a bug in StdLib or simply to that lib not being compiled with UBSan... Any idea how to overcome this, and let the tests pass?
---
[GitHub] thrift pull request #1448: [WIP] Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1448#discussion_r158557742
--- Diff: lib/cpp/src/thrift/processor/TMultiplexedProcessor.h ---
@@ -165,10 +166,11 @@ class TMultiplexedProcessor : public TProcessor {
}
// Extract the service name
- boost::tokenizer<boost::char_separator<char> > tok(name, boost::char_separator<char>(":"));
-
std::vector<std::string> tokens;
- std::copy(tok.begin(), tok.end(), std::back_inserter(tokens));
+ std::istringstream tokenStream(name);
+ for (std::string token; std::getline(tokenStream, token, ':');) {
--- End diff --
getline may only be available in C++11 or later
As a project should we consider calling out 0.11.0 as the last version that will support C++03, and simplify this work?
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1448
Thanks, that's incredibly helpful when it comes to merging it.
---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1448
Is this done (for now) with the exception of that build issue, since you removed the [WIP] tag?
---