Use =default for constructors and operator= where possible#179
Conversation
Codecov Report
@@ Coverage Diff @@
## master #179 +/- ##
==========================================
- Coverage 95.31% 95.19% -0.13%
==========================================
Files 35 35
Lines 3224 3140 -84
==========================================
- Hits 3073 2989 -84
Misses 151 151
Continue to review full report at Codecov.
|
|
Apologies for fixing the noexcept issue piecemeal; I have yet to figure out how to reproduce it in a local build. (What I would give for everything to use a fully hermetic build system?!) |
NoteGiven that this PR consist mostly of removing code, I suspect the coverage numbers are not meaningful. Also, the delta is of a nature where most possibilities for failure are compile time issues and as such will be detected regardless of if the tests cases cover them. |
etr
left a comment
etr
left a comment
There was a problem hiding this comment.
Not worried about the reduction in coverage as it is just the effect of the change in proportion between code ignored/considered by the metric.
| std::map<std::string, std::string, http::header_comparator> cookies; | ||
|
|
||
| friend std::ostream &operator<< (std::ostream &os, const http_response &r); | ||
| friend std::ostream &operator<< (std::ostream &os, const http_response &r); |
There was a problem hiding this comment.
the indentation must have accidentally changed here. Can we fix it?
| size_t _content_size_limit = std::numeric_limits<size_t>::max(); | ||
| int _connection_timeout = DEFAULT_WS_TIMEOUT; | ||
| int _per_IP_connection_limit = 0; | ||
| log_access_ptr _log_access = nullptr; |
There was a problem hiding this comment.
As there are many comparisons of these values to 0x0, I'd isolate the change to nullptr from this change. At least we keep the change dedicated to only moving to default constructor.
Also, despite nullptr is 0x0 under the hood, but formally I'd rather have that consistent before changing it as it should act as an abstraction (so I'd rather not assume it is).
| int _max_threads = 0; | ||
| int _max_connections = 0; | ||
| int _memory_limit = 0; | ||
| size_t _content_size_limit = std::numeric_limits<size_t>::max(); |
There was a problem hiding this comment.
The change from 0 to max is definitely a correct change, but I'd rather do it in a different CR to push before pushing this.
There was a problem hiding this comment.
Mind if I do it after this one?
(Git seems to be gratuitously obtuse when it comes to dealing with merges and PRs.)
|
The change looks good. There have been some uncorrelated failures on Travis (they were having problems at connecting to PPA). I am restarting those to make sure nothing happens there and approving this change. |
|
Merged. Thanks for all the work and support. |
3 participants
Identify the Bug
#177 (General code style issues)
Description of the Change
Use
=defaultfor constructors andoperator=where possible.For default construction, this moves the default values to a default member initializer:
https://en.cppreference.com/w/cpp/language/data_members#Member_initialization
This significantly reduce the size of code in a number of places. It also avoids the need to ensure that the list of copied/moved fields matches the list of existing fields (in some case it did not).
Possible Drawbacks
This removes the "effectively unused" protected member http_response::content. If any client code references that, this will break it.
Verification Process
Release Notes
Use
=defaultfor constructors andoperator=where possible.