Missing operators and ctors for http_request and http_response #1668
Conversation
This file contains 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
barcharcraz
requested changes
Dec 16, 2021
Release/include/cpprest/http_msg.h
Outdated
Comment on lines
553
to
605
| /// <summary> | ||
| /// Constructs a response object | ||
| /// </summary> | ||
| /// <returns>A new HTTP response.</returns> | ||
| http_response(const http_response& _Other) : _m_impl(_Other._m_impl) {} | ||
|
|
||
| /// <summary> | ||
| /// Constructs a response object | ||
| /// </summary> | ||
| /// <returns>A new HTTP response.</returns> | ||
| http_response(http_response&& _Other) : _m_impl(std::move(_Other._m_impl)) {} | ||
|
|
||
| /// <summary>Replaces the contents of one <c>http_request</c> object with another.</summary> | ||
| /// <param name="_Other">The source <c>http_request</c> object.</param> | ||
| /// <remarks> | ||
| /// As <c>http_request</c> behaves like a smart pointer, after a copy assignment, this <c>http_request</c> | ||
| /// objects represents the same actual http_request as <paramref name="_Other"/> does. | ||
| /// </remarks> | ||
| /// <returns>A new HTTP response.</returns> | ||
| http_response& operator=(const http_response& _Other) | ||
| { | ||
| if (this != &_Other) | ||
| { | ||
| _m_impl = _Other._m_impl; | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Destructor frees any held resources. | ||
| /// </summary> | ||
| ~http_response() = default; | ||
|
|
||
| /// <summary>Replaces the contents of one <c>http_request</c> object with another.</summary> | ||
| /// <param name="_Other">The source <c>http_request</c> object.</param> | ||
| /// <remarks> | ||
| /// As <c>http_request</c> behaves like a smart pointer, after a copy assignment, this <c>http_request</c> | ||
| /// objects represents the same actual http_request as <paramref name="_Other"/> does. | ||
| /// </remarks> | ||
| /// <returns>A new HTTP response.</returns> | ||
| http_response& operator=(http_response&& _Other) | ||
| { | ||
| if (this != &_Other) | ||
| { | ||
| _m_impl = std::move(_Other._m_impl); | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Destructor frees any held resources. | ||
| /// </summary> | ||
| ~http_response() = default; |
There was a problem hiding this comment.
These don't appear to do anything over just accepting the compiler's defaults. The original class has an empty destructor, which is somewhat redundant, not an excuse to add even more redundant stuff
| @@ -853,7 +909,7 @@ class _http_request final : public http::details::http_msg_base, public std::ena | |||
|
|
|||
| _ASYNCRTIMP _http_request(std::unique_ptr<http::details::_http_server_context> server_context); | |||
|
|
|||
| virtual ~_http_request() {} | |||
| virtual ~_http_request() = default; | |||
Release/include/cpprest/http_msg.h
Outdated
Comment on lines
1010
to
1070
| /// <summary> | ||
| /// Constructs a <c>http_request</c> object. | ||
| /// </summary> | ||
| /// <param name="_Other"> | ||
| /// The source <c>http_request</c> object. | ||
| /// </param> | ||
| http_request(const http_request& _Other) : _m_impl(_Other._m_impl) {} | ||
|
|
||
| /// <summary> | ||
| /// Constructs a <c>http_request</c> object. | ||
| /// </summary> | ||
| /// <param name="_Other"> | ||
| /// The source <c>http_request</c> object. | ||
| /// </param> | ||
| http_request(http_request&& _Other) : _m_impl(_Other._m_impl) {} | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Replaces the contents of one <c>http_request</c> object with another. | ||
| /// </summary> | ||
| /// <param name="_Other"> | ||
| /// The source <c>http_request</c> object. | ||
| /// </param> | ||
| /// <remarks> | ||
| /// As <c>http_request</c> behaves like a smart pointer, after a copy assignment, this <c>http_request</c> | ||
| /// objects represents the same actual http_request as <paramref name="_Other"/> does. | ||
| /// </remarks> | ||
| /**/ | ||
| http_request& operator=(const http_request& _Other) | ||
| { | ||
| if (this != &_Other) | ||
| { | ||
| _m_impl = _Other._m_impl; | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Replaces the contents of one <c>http_request</c> object with another. | ||
| /// </summary> | ||
| /// <param name="_Other"> | ||
| /// The source <c>http_request</c> object. | ||
| /// </param> | ||
| /// <remarks> | ||
| /// As <c>http_request</c> behaves like a smart pointer, after a copy assignment, this <c>http_request</c> | ||
| /// objects represents the same actual http_request as <paramref name="_Other"/> does. | ||
| /// </remarks> | ||
| /**/ | ||
| http_request& operator=(http_request&& _Other) | ||
| { | ||
| if (this != &_Other) | ||
| { | ||
| _m_impl = std::move(_Other._m_impl); | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Destructor frees any held resources. | ||
| /// </summary> | ||
| ~http_request() {} | ||
| ~http_request() = default; |
|
This approach seems completely backwards, for starters it adds a bunch of (implicit) copy constructors and that's totally not necessary, and also, the new move constructor doesn't move! It would be much better to stop defining an empty destructor (which is equivalent to what the compiler would do anyways) and just default the move constructor, so the shared pointer would get moved |
|
Ok. My point was to implement all functions of rule of five to not be aware if compiler won't generate move/copy ctors and assignment operators. If we wan't to produce less redunant code, we must follow standard which says that compiler will generate both copy and assignment functions if we doesn't have user-defined destructor. The main problem is this line. User-defined destructor means that compiler won't support us |
|
Also this file has other ctors and dtors that are defaulted. If it is the matter of style so why we won't change them to {} too? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Solves this issue: #1667