[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
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
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 issue #1448: Thrift-4441: Support compilation without Boost
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 pull request #1448: Thrift-4441: Support compilation without Boost
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
Github user Typz commented on the issue: https://github.com/apache/thrift/pull/1448 @jeking3 : any suggestion? ---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
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
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 > tok(name,
boost::char_separator(":"));
-
std::vector 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: [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 Date: 2017-12-11T17:18:51Z Replace boost::noncopyable with a custom implementation commit 657ba7028a32390a57ed34a66d51963daa50ee56 Author: Francois Ferrand 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 Date: 2017-12-12T09:10:50Z Remove unused boost header inclusion commit de1627987daebb5286998bfc13490be0af4e58a1 Author: Francois Ferrand Date: 2017-12-12T09:28:26Z Replace boost::atomic with std::atomic commit 4c6300a6d381fef36f6a139470c3f094759bb287 Author: Francois Ferrand Date: 2017-12-12T09:49:57Z Replace boost::unique_ptr with std::unique_ptr commit bb1c3e0cd9988efc3f1ede825a24041ea9574f67 Author: Francois Ferrand Date: 2017-12-12T10:22:14Z Replace boost::tokenizer with std::getline commit 939fbe89015bebf2e1223742ca217d1fbb1d0a90 Author: Francois Ferrand Date: 2017-12-12T11:03:28Z Replace BOOST_STATIC_ASSERT with static_assert commit 187487f8eab8b95b921d7ee6030f439ca4182214 Author: Francois Ferrand Date: 2017-12-12T14:08:06Z Use fpclassify and signbit from std lib commit 330d9f812d3a5157349d70ab322dad2715bbc72c Author: Francois Ferrand Date: 2017-12-12T17:10:41Z Use std::codecvt_utf8_utf16 for utf16 to utf8 conversion commit e760eb4231710334b506df353918fbba73e5fe83 Author: Francois Ferrand Date: 2017-12-12T17:32:19Z Replace boost::numeric_cast with std::numeric_limits commit a13452891a4826e255053441a137796222ab9c75 Author: Francois Ferrand Date: 2017-12-14T10:52:16Z Use ostringstream instead of boost::format commit 8fb0c8461ddc974b2a7dc310e79f4501fc30341b Author: Francois Ferrand Date: 2017-12-14T11:32:03Z Replace BOOST_SCOPE_EXIT with unique_ptr and custom deleter commit cfad8e6165d7c18de1f52ea9979a8bc9d1e3c13a Author: Francois Ferrand Date: 2017-12-15T12:46:18Z Remove use of boost::istarts_with and boost::iends_with ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1353#discussion_r138849538
--- Diff: lib/cpp/src/thrift/concurrency/TimerManager.h ---
@@ -69,28 +72,33 @@ class TimerManager {
*
* @param task The task to execute
* @param timeout Time in milliseconds to delay before executing task
+ * @return Handle of the timer, which can be used to remose the timer.
--- End diff --
Done
---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1353#discussion_r138849560
--- Diff: lib/cpp/test/concurrency/TimerManagerTests.h ---
@@ -192,6 +192,38 @@ class TimerManagerTests {
return true;
}
+/**
+ * This test creates two tasks, removes the first one then waits for the
second one. It then
+ * verifies that the timer manager properly clean up itself and the
remaining orphaned timeout
+ * task when the manager goes out of scope and its destructor is called.
+ */
+ bool test03(int64_t timeout = 1000LL) {
+TimerManager timerManager;
+timerManager.threadFactory(shared_ptr(new
PlatformThreadFactory()));
+timerManager.start();
+assert(timerManager.state() == TimerManager::STARTED);
+
+Synchronized s(_monitor);
+
+// Setup the two tasks
+shared_ptr taskToRemove
+= shared_ptr(new
TimerManagerTests::Task(_monitor, timeout / 2));
+TimerManager::Timer timer = timerManager.add(taskToRemove,
taskToRemove->_timeout);
+
+shared_ptr task
+ = shared_ptr(new
TimerManagerTests::Task(_monitor, timeout));
+timerManager.add(task, task->_timeout);
+
+// Remove one task and wait until the other has completed
+timerManager.remove(timer);
--- End diff --
Done
---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1353#discussion_r138845074
--- Diff: lib/cpp/src/thrift/concurrency/TimerManager.h ---
@@ -100,13 +108,26 @@ class TimerManager {
*/
virtual void remove(stdcxx::shared_ptr task);
+ /**
+ * Removes a single pending task
+ *
+ * @param timer The timer to remove. The timer is returned when calling
the
+ * add() method.
+ * @throws NoSuchTaskException Specified task doesn't exist. It was
either
+ * processed already or this call was made
for a
+ * task that was never added to this timer
+ *
+ * @throws UncancellableTaskException Specified task is already being
+ *executed or has completed
execution.
+ */
+ virtual void remove(Timer timer);
--- End diff --
The old function removes *all* timers executing this task, and does this
inefficiently by traversing the whole list.
This new function removes a single timer in constant time [O(1)] which may
be significant for large system with many timers.
This does not impact the behavior of Thrift itself, since it does not rely
on timer removal.
---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
GitHub user Typz opened a pull request: https://github.com/apache/thrift/pull/1353 THRIFT-4327: add API to efficiently remove a single timer You can merge this pull request into a Git repository by running: $ git pull https://github.com/Typz/thrift THRIFT-4327 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1353.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 #1353 commit d1fd82a84af5346ce8b2c68cd8cb07c6533340a6 Author: Francois Ferrand Date: 2017-09-11T10:09:40Z THRIFT-4327: add API to efficiently remove a single timer ---
[GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1337#discussion_r137811890
--- Diff: lib/cpp/src/thrift/concurrency/TimerManager.cpp ---
@@ -290,11 +292,23 @@ void TimerManager::add(shared_ptr task,
const struct timeval& value) {
}
void TimerManager::remove(shared_ptr task) {
- (void)task;
Synchronized s(monitor_);
if (state_ != TimerManager::STARTED) {
throw IllegalStateException();
}
+ std::vector toRemove;
--- End diff --
done
---
[GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1337#discussion_r137810110
--- Diff: lib/cpp/test/concurrency/TimerManagerTests.h ---
@@ -126,6 +126,38 @@ class TimerManagerTests {
return true;
}
+ /**
+ * This test creates two tasks, removes the first one then waits for the
second one. It then
+ * verifies that the timer manager properly clean up itself and the
remaining orphaned timeout
+ * task when the manager goes out of scope and its destructor is called.
+ */
+ bool test01(int64_t timeout = 1000LL) {
+TimerManager timerManager;
+timerManager.threadFactory(shared_ptr(new
PlatformThreadFactory()));
+timerManager.start();
+assert(timerManager.state() == TimerManager::STARTED);
+
+Synchronized s(_monitor);
+
+// Setup the two tasks
+shared_ptr taskToRemove
+ = shared_ptr(new
TimerManagerTests::Task(_monitor, timeout / 2));
+timerManager.add(taskToRemove, taskToRemove->_timeout);
+
+shared_ptr task
+ = shared_ptr(new
TimerManagerTests::Task(_monitor, timeout));
+timerManager.add(task, task->_timeout);
+
+// Remove one task and wait until the other has completed
+timerManager.remove(taskToRemove);
+_monitor.wait(timeout * 2);
+
+assert(!taskToRemove->_done);
+assert(task->_done);
+
+return true;
+ }
+
--- End diff --
done
---
[GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1337#discussion_r137809132
--- Diff: lib/cpp/src/thrift/concurrency/TimerManager.cpp ---
@@ -52,6 +52,8 @@ class TimerManager::Task : public Runnable {
}
}
+ shared_ptr runnable() const { return runnable_; }
--- End diff --
done
---
[GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()
Github user Typz commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1337#discussion_r137808236
--- Diff: lib/cpp/src/thrift/concurrency/TimerManager.cpp ---
@@ -290,11 +292,23 @@ void TimerManager::add(shared_ptr task,
const struct timeval& value) {
}
void TimerManager::remove(shared_ptr task) {
- (void)task;
Synchronized s(monitor_);
if (state_ != TimerManager::STARTED) {
throw IllegalStateException();
}
+ std::vector toRemove;
+ for (task_iterator ix = taskMap_.begin(); ix != taskMap_.end(); ++ix) {
+if (ix->second->runnable() == task) {
+ toRemove.push_back(ix);
+}
+ }
+ if (toRemove.empty()) {
+throw NoSuchTaskException();
+ }
+ for (std::vector::iterator ix = toRemove.begin(); ix !=
toRemove.end(); ++ix) {
+taskMap_.erase(*ix);
--- End diff --
Tasks cannot exist multiple times, only newly allocated tasks are inserted:
```cpp
taskMap_.insert(
std::pair >(timeout, shared_ptr(new
Task(task;
```
Which is why I implemented it in the same fashion as the run() method. But
i can change it.
---
[GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()
GitHub user Typz opened a pull request: https://github.com/apache/thrift/pull/1337 THRIFT-4292: Implement TimerManager::remove() You can merge this pull request into a Git repository by running: $ git pull https://github.com/Typz/thrift THRIFT-4292 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1337.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 #1337 commit 177c9ea5a246f296857bb62288befa5acc295902 Author: Francois Ferrand Date: 2017-08-25T07:01:26Z THRIFT-4292: Implement TimerManager::remove() --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
