address review comments

Author: epistor

Release/cmake/cpprest_find_brotli.cmake

@@ -3,7 +3,7 @@ function(cpprest_find_brotli)
     return()
   endif()
 
-  find_package(UNOFFICIAL-BROTLI REQUIRED)
+  find_package(unofficial-brotli REQUIRED)
 
   add_library(cpprestsdk_brotli_internal INTERFACE)
   target_link_libraries(cpprestsdk_brotli_internal INTERFACE unofficial::brotli::brotlienc unofficial::brotli::brotlidec unofficial::brotli::brotlicommon)

Release/cmake/cpprestsdk-config.in.cmake

@@ -4,7 +4,7 @@ if(@CPPREST_USES_ZLIB@)
 endif()
 
 if(@CPPREST_USES_BROTLI@)
-  find_dependency(UNOFFICIAL-BROTLI)
+  find_dependency(unofficial-brotli)
 endif()
 
 if(@CPPREST_USES_OPENSSL@)

Release/include/cpprest/http_compression.h

@@ -115,24 +115,20 @@ _ASYNCRTIMP bool supported();
 /// <summary>
 // String constants for each built-in compression algorithm, for convenient use with the factory functions
 /// </summary>
-class algorithm
+namespace algorithm
 {
-public:
-    _ASYNCRTIMP static const utility::string_t GZIP;
-    _ASYNCRTIMP static const utility::string_t DEFLATE;
-    _ASYNCRTIMP static const utility::string_t BROTLI;
-
-    /// <summary>
-    /// Test whether cpprestsdk was built with built-in compression support and
-    /// the supplied string matches a supported built-in algorithm
-    /// <param name="algorithm">The name of the algorithm to test for built-in support.</param>
-    /// <returns>True if cpprestsdk was built with built-in compression support and
-    /// the supplied string matches a supported built-in algorithm, and false if not.</returns>
-    /// <summary>
-    _ASYNCRTIMP static bool supported(const utility::string_t& algorithm);
+constexpr utility::char_t *GZIP = _XPLATSTR("gzip");
+constexpr utility::char_t *DEFLATE = _XPLATSTR("deflate");
+constexpr utility::char_t *BROTLI = _XPLATSTR("br");
 
-private:
-    algorithm() {}
+/// <summary>
+/// Test whether cpprestsdk was built with built-in compression support and
+/// the supplied string matches a supported built-in algorithm
+/// <param name="algorithm">The name of the algorithm to test for built-in support.</param>
+/// <returns>True if cpprestsdk was built with built-in compression support and
+/// the supplied string matches a supported built-in algorithm, and false if not.</returns>
+/// <summary>
+_ASYNCRTIMP bool supported(const utility::string_t& algorithm);
 };
 
 /// <summary>
@@ -255,12 +251,6 @@ _ASYNCRTIMP std::shared_ptr<decompress_factory> make_decompress_factory(
 
 namespace details
 {
-namespace builtin
-{
-// Internal-only helper function
-const std::vector<std::shared_ptr<decompress_factory>> get_decompress_factories();
-} // namespace builtin
-
 /// <summary>
 /// Header type enum for use with compressor and decompressor header parsing and building functions
 /// </summary>

Release/include/cpprest/http_headers.h

@@ -219,7 +219,7 @@ class http_headers
             return false;
         }
 
-        return bind_impl(iter->second, value) || iter->second.empty();
+        return details::bind_impl(iter->second, value) || iter->second.empty();
     }
 
     /// <summary>
@@ -285,9 +285,14 @@ class http_headers
     _ASYNCRTIMP void set_date(const utility::datetime& date);
 
 private:
+    // Headers are stored in a map with case insensitive key.
+    inner_container m_headers;
+};
 
-    template<typename _t>
-    bool bind_impl(const key_type &text, _t &ref) const
+namespace details
+{
+    template<typename key_type, typename _t>
+    bool bind_impl(const key_type &text, _t &ref)
     {
         utility::istringstream_t iss(text);
         iss.imbue(std::locale::classic());
@@ -300,20 +305,18 @@ class http_headers
         return true;
     }
 
-    bool bind_impl(const key_type &text, utf16string &ref) const
+    template<typename key_type>
+    bool bind_impl(const key_type &text, utf16string &ref)
     {
         ref = utility::conversions::to_utf16string(text);
         return true;
     }
 
-    bool bind_impl(const key_type &text, std::string &ref) const
+    template<typename key_type>
+    bool bind_impl(const key_type &text, std::string &ref)
     {
         ref = utility::conversions::to_utf8string(text);
         return true;
     }
-
-    // Headers are stored in a map with case insensitive key.
-    inner_container m_headers;
-};
-
+}
 }}

Release/include/cpprest/http_msg.h

@@ -383,7 +383,7 @@ class http_msg_base
     /// Determine the remaining input stream length
     /// </summary>
     /// <returns>
-    /// size_t::max if the stream's remaining length cannot be determined
+    /// std::numeric_limits<size_t>::max() if the stream's remaining length cannot be determined
     /// length      if the stream's remaining length (which may be 0) can be determined
     /// </returns>
     /// <remarks>
@@ -396,7 +396,7 @@ class http_msg_base
     /// Determine the content length
     /// </summary>
     /// <returns>
-    /// size_t::max if there is content with unknown length (transfer_encoding:chunked)
+    /// std::numeric_limits<size_t>::max() if there is content with unknown length (transfer_encoding:chunked)
     /// 0           if there is no content
     /// length      if there is content with known length
     /// </returns>
@@ -410,7 +410,7 @@ class http_msg_base
     /// Determine the content length, and, if necessary, manage compression in the Transfer-Encoding header
     /// </summary>
     /// <returns>
-    /// size_t::max if there is content with unknown length (transfer_encoding:chunked)
+    /// std::numeric_limits<size_t>::max() if there is content with unknown length (transfer_encoding:chunked)
     /// 0           if there is no content
     /// length      if there is content with known length
     /// </returns>
@@ -739,7 +739,7 @@ class http_response
     /// <param name="stream">A readable, open asynchronous stream.</param>
     /// <param name="content_type">A string holding the MIME type of the message body.</param>
     /// <remarks>
-    /// This cannot be used in conjunction with any other means of setting the body of the request.
+    /// This cannot be used in conjunction with any external means of setting the body of the request.
     /// The stream will not be read until the message is sent.
     /// </remarks>
     void set_body(const concurrency::streams::istream &stream, const utility::string_t &content_type = _XPLATSTR("application/octet-stream"))
@@ -755,7 +755,7 @@ class http_response
     /// <param name="content_length">The size of the data to be sent in the body.</param>
     /// <param name="content_type">A string holding the MIME type of the message body.</param>
     /// <remarks>
-    /// This cannot be used in conjunction with any other means of setting the body of the request.
+    /// This cannot be used in conjunction with any external means of setting the body of the request.
     /// The stream will not be read until the message is sent.
     /// </remarks>
     void set_body(const concurrency::streams::istream &stream, utility::size64_t content_length, const utility::string_t &content_type = _XPLATSTR("application/octet-stream"))
@@ -1222,7 +1222,7 @@ class http_request
     /// </summary>
     /// <param name="compressor">A pointer to an instantiated compressor of the desired type.</param>
     /// <remarks>
-    /// This cannot be used in conjunction with any other means of compression.  The Transfer-Encoding
+    /// This cannot be used in conjunction with any external means of compression.  The Transfer-Encoding
     /// header will be managed internally, and must not be set by the client.
     /// </remarks>
     void set_compressor(std::unique_ptr<http::compression::compress_provider> compressor)
@@ -1238,7 +1238,7 @@ class http_request
     /// True if a built-in compressor was instantiated, otherwise false.
     /// </returns>
     /// <remarks>
-    /// This cannot be used in conjunction with any other means of compression.  The Transfer-Encoding
+    /// This cannot be used in conjunction with any external means of compression.  The Transfer-Encoding
     /// header will be managed internally, and must not be set by the client.
     /// </remarks>
     bool set_compressor(utility::string_t algorithm)
@@ -1297,7 +1297,7 @@ class http_request
     /// The collection of factory classes itself.
     /// </returns>
     /// <remarks>
-    /// This cannot be used in conjunction with any other means of decompression.  The TE
+    /// This cannot be used in conjunction with any external means of decompression.  The TE
     /// header must not be set by the client, as it will be managed internally.
     /// </remarks>
     const std::vector<std::shared_ptr<http::compression::decompress_factory>> &decompress_factories() const

Release/src/CMakeLists.txt

@@ -72,6 +72,9 @@ endif()
 
 # Compression component
 if(CPPREST_EXCLUDE_COMPRESSION)
+  if(NOT CPPREST_EXCLUDE_BROTLI)
+    message(FATAL_ERROR "Use of Brotli requires compression to be enabled")
+  endif()
   target_compile_definitions(cpprest PRIVATE -DCPPREST_EXCLUDE_COMPRESSION=1)
 else()
   cpprest_find_zlib()

Release/src/http/client/http_client_asio.cpp

@@ -1520,29 +1520,26 @@ class asio_context final : public request_context, public std::enable_shared_fro
 
     bool decompress(const uint8_t* input, size_t input_size, std::vector<uint8_t>& output)
     {
-        size_t processed;
-        size_t got;
-        size_t inbytes;
-        size_t outbytes;
-        bool done;
-
         // Need to guard against attempting to decompress when we're already finished or encountered an error!
         if (input == nullptr || input_size == 0)
         {
             return false;
         }
 
-        inbytes = 0;
-        outbytes = 0;
-        done = false;
+        size_t processed;
+        size_t got;
+        size_t inbytes = 0;
+        size_t outbytes = 0;
+        bool done = false;
+
         try
         {
             output.resize(input_size * 3);
             do
             {
                 if (inbytes)
                 {
-                    output.resize(output.size() + (input_size > 1024 ? input_size : 1024));
+                    output.resize(output.size() + std::max(input_size, static_cast<size_t>(1024)));
                 }
                 got = m_decompressor->decompress(input + inbytes,
                                                  input_size - inbytes,

Release/src/http/client/http_client_winhttp.cpp

@@ -182,10 +182,10 @@ class memory_holder
 {
     uint8_t* m_externalData;
     std::vector<uint8_t> m_internalData;
-    size_t m_length;
+    size_t m_size;
 
 public:
-    memory_holder() : m_externalData(nullptr), m_length(0)
+    memory_holder() : m_externalData(nullptr), m_size(0)
     {
     }
 
@@ -202,7 +202,7 @@ class memory_holder
     {
         assert(block != nullptr);
         m_externalData = block;
-        m_length = length;
+        m_size = length;
     }
 
     inline bool is_internally_allocated() const
@@ -215,9 +215,9 @@ class memory_holder
         return is_internally_allocated() ? &m_internalData[0] : m_externalData ;
     }
 
-    inline size_t length() const
+    inline size_t size() const
     {
-        return is_internally_allocated() ? m_internalData.size() : m_length;
+        return is_internally_allocated() ? m_internalData.size() : m_size;
     }
 };
 
@@ -1311,13 +1311,7 @@ class winhttp_client final : public _http_client_communicator
             {
                 uint8_t *buffer;
 
-                if (!decompressor)
-                {
-                    auto writebuf = pContext->_get_writebuffer();
-                    pContext->allocate_reply_space(writebuf.alloc(chunkSize), chunkSize);
-                    buffer = pContext->m_body_data.get();
-                }
-                else
+                if (decompressor)
                 {
                     // m_buffer holds the compressed data; we'll decompress into the caller's buffer later
                     if (pContext->m_compression_state.m_buffer.capacity() < chunkSize)
@@ -1326,6 +1320,12 @@ class winhttp_client final : public _http_client_communicator
                     }
                     buffer = pContext->m_compression_state.m_buffer.data();
                 }
+                else
+                {
+                    auto writebuf = pContext->_get_writebuffer();
+                    pContext->allocate_reply_space(writebuf.alloc(chunkSize), chunkSize);
+                    buffer = pContext->m_body_data.get();
+                }
 
                 if (!WinHttpReadData(
                     pContext->m_request_handle,
@@ -1350,15 +1350,15 @@ class winhttp_client final : public _http_client_communicator
         {
             // We could allocate less than a chunk for the compressed data here, though that
             // would result in more trips through this path for not-so-compressible data...
-            if (p_request_context->m_body_data.length() > http::details::chunked_encoding::additional_encoding_space)
+            if (p_request_context->m_body_data.size() > http::details::chunked_encoding::additional_encoding_space)
             {
                 // If we've previously allocated space for the compressed data, don't reduce it
-                chunk_size = p_request_context->m_body_data.length() - http::details::chunked_encoding::additional_encoding_space;
+                chunk_size = p_request_context->m_body_data.size() - http::details::chunked_encoding::additional_encoding_space;
             }
             else if (p_request_context->m_remaining_to_write != std::numeric_limits<size_t>::max())
             {
                 // Choose a semi-intelligent size based on how much total data is left to compress
-                chunk_size = std::min((size_t)p_request_context->m_remaining_to_write+128, p_request_context->m_http_client->client_config().chunksize());
+                chunk_size = std::min(static_cast<size_t>(p_request_context->m_remaining_to_write)+128, p_request_context->m_http_client->client_config().chunksize());
             }
             else
             {
@@ -1369,7 +1369,7 @@ class winhttp_client final : public _http_client_communicator
         else
         {
             // We're not compressing; use the smaller of the remaining data (if known) and the configured (or default) chunk size
-            chunk_size = std::min((size_t)p_request_context->m_remaining_to_write, p_request_context->m_http_client->client_config().chunksize());
+            chunk_size = std::min(static_cast<size_t>(p_request_context->m_remaining_to_write), p_request_context->m_http_client->client_config().chunksize());
         }
         p_request_context->allocate_request_space(nullptr, chunk_size + http::details::chunked_encoding::additional_encoding_space);
 
@@ -1590,7 +1590,7 @@ class winhttp_client final : public _http_client_communicator
                 }
                 else
                 {
-                    length = std::min((size_t)p_request_context->m_remaining_to_write, p_request_context->m_http_client->client_config().chunksize());
+                    length = std::min(static_cast<size_t>(p_request_context->m_remaining_to_write), p_request_context->m_http_client->client_config().chunksize());
                     if (p_request_context->m_compression_state.m_buffer.capacity() < length)
                     {
                         p_request_context->m_compression_state.m_buffer.reserve(length);
@@ -2182,7 +2182,7 @@ class winhttp_client final : public _http_client_communicator
 
                 if (p_request_context->m_decompressor)
                 {
-                    size_t chunk_size = std::max((size_t)bytesRead, p_request_context->m_http_client->client_config().chunksize());
+                    size_t chunk_size = std::max(static_cast<size_t>(bytesRead), p_request_context->m_http_client->client_config().chunksize());
                     p_request_context->m_compression_state.m_bytes_read = static_cast<size_t>(bytesRead);
                     p_request_context->m_compression_state.m_chunk_bytes = 0;