http: Do a pending c++11 simplification handling work items #7966
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -71,8 +71,7 @@ class WorkQueue | |||
| /** Mutex protects entire object */ | |||
| CWaitableCriticalSection cs; | |||
| CConditionVariable cond; | |||
| /* XXX in C++11 we can use std::unique_ptr here and avoid manual cleanup */ | |||
| std::deque<WorkItem*> queue; | |||
| std::deque<std::unique_ptr<WorkItem> > queue; | |||
There was a problem hiding this comment.
Also, in C++11, you no longer need to avoid the '>>' in Template1<Template2<Template3>>.
There was a problem hiding this comment.
I was just about to ask why the space is there...
There was a problem hiding this comment.
@paveljanik because >> is an operator. But yes with c++11 this is apparently parsed differently so will remove the space.
c0c7d8b to
3fc99f4
Compare
| @@ -106,19 +105,15 @@ class WorkQueue | |||
| */ | |||
| ~WorkQueue() | |||
There was a problem hiding this comment.
Do we need an empty destructor?
There was a problem hiding this comment.
On second thought, I'm going to keep it, the precondition comment is still relevant.
fa1b71a to
d16d992
Compare
|
Decided to forget about using std::move to move items conditionally into the queue, the resulting code was too complex, the old code is cleaner. |
827a36f to
867bc6d
Compare
| @@ -118,7 +112,7 @@ class WorkQueue | |||
| if (queue.size() >= maxDepth) { | |||
| return false; | |||
| } | |||
| queue.push_back(item); | |||
| queue.push_back(std::unique_ptr<WorkItem>(item)); | |||
|
ut ACK, other than the little nit |
Use std::unique_ptr for handling work items. This makes the code more RAII and, as mentioned in the comment, is what I planned when I wrote the code in the first place.
More useful error reporting.
No need for boost here.
Thanks to Cory Fields for the idea.
867bc6d to
f0188f9
Compare
|
Fixed @theuni's nit, thanks |
laanwj
added a commit
that referenced
this pull request
May 5, 2016
f0188f9 http: use std::move to move HTTPRequest into HTTPWorkItem (Wladimir J. van der Laan) 37b2137 http: Change boost::scoped_ptr to std::unique_ptr in HTTPRequest (Wladimir J. van der Laan) f97b410 http: Add log message when work queue is full (Wladimir J. van der Laan) 091d6e0 http: Do a pending c++11 simplification (Wladimir J. van der Laan)
| @@ -118,7 +112,7 @@ class WorkQueue | |||
| if (queue.size() >= maxDepth) { | |||
| return false; | |||
| } | |||
| queue.push_back(item); | |||
| queue.emplace_back(std::unique_ptr<WorkItem>(item)); | |||
There was a problem hiding this comment.
Creating the unique_ptr here defeats the purpose of the emplace :)
Passing it item directly would mean that the unique_ptr would be constructed in-place. As is, it's moved instead.
Not worth worrying about, just pointing it out.
There was a problem hiding this comment.
Yes by all means feel free to improve this further.
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Dec 21, 2017
…work items f0188f9 http: use std::move to move HTTPRequest into HTTPWorkItem (Wladimir J. van der Laan) 37b2137 http: Change boost::scoped_ptr to std::unique_ptr in HTTPRequest (Wladimir J. van der Laan) f97b410 http: Add log message when work queue is full (Wladimir J. van der Laan) 091d6e0 http: Do a pending c++11 simplification (Wladimir J. van der Laan)
sickpig
pushed a commit
to sickpig/BitcoinUnlimited
that referenced
this pull request
Mar 12, 2018
HTTP Server cherries from Core: bitcoin/bitcoin#6719 - Make HTTP server shutdown more graceful bitcoin/bitcoin#6859 - http: Restrict maximum size of http + headers bitcoin/bitcoin#6990 - http: speed up shutdown bitcoin/bitcoin#7966 - http: Do a pending c++11 simplification handling work items bitcoin/bitcoin#8421 - httpserver: drop boost (#8023 dependency) bitcoin/bitcoin#11006 - Improve shutdown process
marlengit
pushed a commit
to marlengit/BitcoinUnlimited
that referenced
this pull request
Sep 25, 2018
HTTP Server cherries from Core: bitcoin/bitcoin#6719 - Make HTTP server shutdown more graceful bitcoin/bitcoin#6859 - http: Restrict maximum size of http + headers bitcoin/bitcoin#6990 - http: speed up shutdown bitcoin/bitcoin#7966 - http: Do a pending c++11 simplification handling work items bitcoin/bitcoin#8421 - httpserver: drop boost (#8023 dependency) bitcoin/bitcoin#11006 - Improve shutdown process
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Oct 1, 2020
Small httpserver.cpp backports Also includes a change to the `uiInterface.NotifyBlockTip` signal API. These remove merge conflicts from subsequent backports for `sync.h`. Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6859 - bitcoin/bitcoin#7112 - Only the non-QT changes. - bitcoin/bitcoin#7966 - bitcoin/bitcoin#8421 - We already backported the second commit in #2555
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
std::unique_ptrfor handling http work items, andstd::moveto move them into and out of the queue.This makes the code more RAII and, as mentioned in the comment, is what I planned when I wrote the code in the first place.