Move zlib and brotli compression services from capy to http #250
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds Brotli and Zlib compression subsystems (headers, implementations, CMake/Boost.Build wiring, docs, and tests); migrates internal uses from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client"
participant Serializer as "Serializer" rect rgba(135,206,250,0.5)
participant Polystore as "capy::polystore" rect rgba(144,238,144,0.5)
participant EncodeSvc as "boost::http::brotli::encode_service" rect rgba(255,182,193,0.5)
participant BrotliLib as "libbrotli (C API)" rect rgba(255,228,181,0.5)
Client->>Serializer: provide data to serialize
Serializer->>Polystore: request encode service
Polystore-->>EncodeSvc: return encode_service implementation
Serializer->>EncodeSvc: compress / compress_stream (input)
EncodeSvc->>BrotliLib: BrotliEncoderCompress / BrotliEncoderCompressStream
BrotliLib-->>EncodeSvc: compressed bytes / status
EncodeSvc-->>Serializer: provide compressed output
Serializer-->>Client: send compressed payload
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
MungoG
force-pushed
the
zlib-brotli-migration
branch
from
6d7343a to
a0a3ffa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (2)
271-274: Misleading step name: "Clone Boost.Corosio" actually cloneshttp-root.The step name suggests cloning the Corosio repository, but the
pathishttp-rootwhich is the HTTP module being tested. This appears to be a copy-paste artifact. Consider renaming to "Clone Boost.Http" for consistency with similar steps elsewhere in the file (e.g., line 556).Suggested fix
- - name: Clone Boost.Corosio + - name: Clone Boost.Http uses: actions/checkout@v4 with: path: http-root
527-531: Same misleading step name in the changelog job.This step name also says "Clone Boost.Corosio" but it clones the current repository (http module). Consider renaming to "Clone Boost.Http" for accuracy.
🤖 Fix all issues with AI agents
In `@build/Jamfile`:
- Around line 63-95: The external compression libs are only in requirements and
therefore don't propagate to dependents; update the usage-requirements for lib
boost_http_zlib and lib boost_http_brotli so the external libraries are
transitively exposed: for boost_http_zlib add <library>/zlib//zlib to its
usage-requirements alongside <define>BOOST_HTTP_HAS_ZLIB, and for
boost_http_brotli add <library>/brotli//brotlicommon,
<library>/brotli//brotlidec and <library>/brotli//brotlienc to its
usage-requirements alongside <define>BOOST_HTTP_HAS_BROTLI so linking consumers
get the needed link libraries.
In `@CMakeLists.txt`:
- Around line 183-214: The CMake rules currently link ZLIB and Brotli as PRIVATE
to boost_http_zlib and boost_http_brotli, which prevents consumers from
inheriting those linker dependencies; update the two target_link_libraries calls
so dependencies are PUBLIC: change target_link_libraries(boost_http_zlib PRIVATE
ZLIB::ZLIB) to target_link_libraries(boost_http_zlib PUBLIC ZLIB::ZLIB) and
change target_link_libraries(boost_http_brotli PRIVATE Brotli::common
Brotli::decoder Brotli::encoder) to target_link_libraries(boost_http_brotli
PUBLIC Brotli::common Brotli::decoder Brotli::encoder) so that consumers of
Boost::http_zlib and Boost::http_brotli receive the necessary linker flags for
static linking.
In `@doc/modules/ROOT/pages/compression/brotli.adoc`:
- Around line 76-84: Update the example to stop passing a dictionary to the
service-install functions: remove the second argument from
brotli::install_encode_service and brotli::install_decode_service so each is
called with only the context parameter (capy::polystore& ctx) as their
signatures expect; ensure the load_dictionary call can remain or be removed as
appropriate, but do not pass dict into install_encode_service or
install_decode_service.
- Around line 26-38: Update the example code to use the actual service-based
API: replace any uses of encoder.encode()/decoder.decode() with
svc.compress_stream()/svc.decompress_stream(), replace encoder.set_quality()
with svc.set_parameter(encoder_parameter::quality, ...), and create
encoder/decoder state via svc.create_instance(); remove calls to the
non-existent brotli::encodeBound() and instead allocate buffers based on
streaming patterns or example in test/unit/compression.cpp; and update
control-flow types to use encoder_operation and decoder_result where the example
previously expected encode_result/decoder_result types.
In `@include/boost/http/brotli.hpp`:
- Around line 21-32: The code example in include/boost/http/brotli.hpp uses the
wrong type for the context; replace the example's boost::capy::datastore with
capy::polystore so it matches actual usage; update the snippet that creates the
context and the subsequent calls to install_encode_service and
install_decode_service (references: install_encode_service,
install_decode_service) to use capy::polystore for correctness.
In `@include/boost/http/brotli/error.hpp`:
- Around line 36-41: Update the two truncated enum identifiers to match Brotli
upstream: rename format_exuberant_meta_nibbl to format_exuberant_meta_nibble and
rename format_context_map_repea to format_context_map_repeat so the enum values
(used in BrotliDecoderErrorCode mappings) match the official Brotli constant
names; locate these symbols in the enum declaration in error.hpp and correct the
spelling only, leaving surrounding values and ordering unchanged.
In `@include/boost/http/zlib.hpp`:
- Around line 22-32: The doc example uses the wrong type; replace the datastore
usage with the actual project polystore type by changing the include and
variable type to capy::polystore and construct a capy::polystore named (e.g.)
ctx, then pass that to boost::http::zlib::install_deflate_service and
boost::http::zlib::install_inflate_service (use the capy::polystore symbol
instead of boost::capy::datastore and update the include to the header that
provides capy::polystore).
In `@include/boost/http/zlib/inflate.hpp`:
- Around line 35-39: The example uses the wrong type for the context variable;
change the example to construct a capy::polystore and pass that to
install_inflate_service so the sample matches the function signature: replace
boost::capy::datastore ctx; with boost::capy::polystore ctx; and ensure the
example calls boost::http::zlib::install_inflate_service(ctx) (same fix should
be applied consistently in related docs like deflate.hpp and brotli headers to
match tests that use capy::polystore and the install_inflate_service signature).
In `@src_zlib/stream_cast.hpp`:
- Around line 56-111: The fallback stream_cast_impl currently sets pzs_ to
reinterpret_cast<z_stream_s*>(&st), causing writes to the wrong object; update
the constructor so pzs_ points to the internal zs_ (i.e., initialize pzs_ to
&zs_ instead of &st) and ensure get(), destructor and member declarations
continue to use pzs_, zs_ and st_ so zlib writes go to zs_ and then the
destructor copies those updated fields back into stream st_; adjust any
cast/initialization logic in stream_cast_impl's constructor accordingly.
In `@src/serializer.cpp`:
- Around line 26-32: Wrap the optional Brotli and Zlib includes and any code
that depends on them with the corresponding feature macros: surround inclusion
of "brotli_filter_base.hpp" and Boost Brotli headers with `#ifdef`
BOOST_HTTP_HAS_BROTLI ... `#endif` and similarly surround "zlib_filter_base.hpp"
and the Boost Zlib headers with `#ifdef` BOOST_HTTP_HAS_ZLIB ... `#endif`; also
guard the definitions/implementations of the brotli_filter and zlib_filter
classes (and any functions or methods that reference Boost::http::brotli or
Boost::http::zlib symbols) with the same macros so the file compiles when
Brotli/Zlib are not found.
In `@test/unit/brotli.cpp`:
- Around line 20-41: Replace the TODO placeholders with real assertions: in
test_error_code assert that boost::system::error_code ec constructed with
brotli::error::no_error compares equal to brotli::error::no_error (or
ec.value()==0); in test_decode after creating capy::polystore ctx and calling
brotli::install_decode_service(ctx) assert that the decode service can be
retrieved from ctx (e.g., ctx.get_service<...>() is non-null/has expected type);
similarly in test_encode assert that after brotli::install_encode_service(ctx)
the encode service is retrievable from ctx. Reference test functions
test_error_code, test_decode, test_encode and symbols brotli::error::no_error,
capy::polystore, brotli::install_decode_service, brotli::install_encode_service.
🧹 Nitpick comments (8)
cmake/FindBrotli.cmake (1)
35-43: Add missing inter-library dependencies for Brotli targets.The Brotli decoder and encoder libraries depend on the common library. Without declaring this dependency via
INTERFACE_LINK_LIBRARIES, consumers linking only toBrotli::decoderorBrotli::encoderwill encounter linker errors for missing symbols.Suggested fix
add_library(Brotli::decoder UNKNOWN IMPORTED) set_target_properties(Brotli::decoder PROPERTIES IMPORTED_LOCATION ${Brotli_DEC_LIBRARY} - INTERFACE_INCLUDE_DIRECTORIES ${Brotli_INCLUDE_DIR}) + INTERFACE_INCLUDE_DIRECTORIES ${Brotli_INCLUDE_DIR} + INTERFACE_LINK_LIBRARIES Brotli::common) add_library(Brotli::encoder UNKNOWN IMPORTED) set_target_properties(Brotli::encoder PROPERTIES IMPORTED_LOCATION ${Brotli_ENC_LIBRARY} - INTERFACE_INCLUDE_DIRECTORIES ${Brotli_INCLUDE_DIR}) + INTERFACE_INCLUDE_DIRECTORIES ${Brotli_INCLUDE_DIR} + INTERFACE_LINK_LIBRARIES Brotli::common)include/boost/http/zlib/inflate.hpp (1)
81-86: Add a virtual destructor to the public interface.Polymorphic public types should typically declare a virtual destructor to avoid UB if ever destroyed via base pointer (or explicitly document that deletion isn’t intended).
♻️ Proposed fix
struct BOOST_SYMBOL_VISIBLE inflate_service { + virtual ~inflate_service() = default; /** Return the ZLib version string. */ virtual char const* version() const noexcept = 0;src_brotli/shared_dictionary.cpp (1)
13-15: Placeholder implementation with disabled functionality.The
#if 0blocks disable the Brotli shared dictionary include and all virtual method implementations. The class currently has no active overrides of the base class's pure virtual methods. This appears to be intentional scaffolding for future implementation.If this is meant to be merged in an incomplete state, consider adding a TODO comment explaining what's pending, or track completion in an issue.
Would you like me to open an issue to track the completion of this shared dictionary service implementation?
Also applies to: 36-71
include/boost/http/brotli/shared_dictionary.hpp (1)
43-64: Service interface has no active members.The
shared_dictionary_servicestruct is currently empty since all virtual methods are disabled with#if 0. This means:
- No virtual destructor is declared
- No abstract interface for derived classes to implement
When re-enabling the virtual methods, consider adding a virtual destructor for proper polymorphic deletion:
Suggested addition when enabling the interface
struct BOOST_SYMBOL_VISIBLE shared_dictionary_service { + virtual ~shared_dictionary_service() = default; + `#if` 0 virtual shared_dictionary* create_instance(test/unit/zlib.cpp (1)
20-25: Unused variable in test - consider adding a basic assertion.The
ecvariable is created but never used. While this serves as a compile-time smoke test for the error code conversion, adding a minimal assertion would strengthen the test:Suggested improvement
void test_error_code() { - // TODO boost::system::error_code ec{ zlib::error::buf_err }; + BOOST_TEST(ec == zlib::error::buf_err); + BOOST_TEST(ec.category().name() != nullptr); }include/boost/http/brotli/decode.hpp (1)
13-16: Includetypes.hppdirectly to avoid transitive dependencies.This header uses
alloc_funcandfree_func, so it should include the header that defines them to compile standalone.📎 Proposed fix
`#include` <boost/http/detail/config.hpp> `#include` <boost/http/brotli/error.hpp> `#include` <boost/http/brotli/shared_dictionary.hpp> +#include <boost/http/brotli/types.hpp>include/boost/http/brotli/encode.hpp (2)
13-15: Includetypes.hppdirectly to avoid transitive dependencies.This header uses
alloc_func/free_func, so it should include the defining header explicitly.📎 Proposed fix
`#include` <boost/http/detail/config.hpp> `#include` <boost/http/brotli/shared_dictionary.hpp> +#include <boost/http/brotli/types.hpp>
288-301: Consider const‑correctness for accessors.
is_finishedandhas_more_outputappear to be read‑only; takingconst encoder_state*would align withdecode_serviceand avoid forcing callers to drop const. If these functions mutate state, ignore.♻️ Proposed adjustment
- virtual bool - is_finished(encoder_state* state) const noexcept = 0; + virtual bool + is_finished(const encoder_state* state) const noexcept = 0; - virtual bool - has_more_output(encoder_state* state) const noexcept = 0; + virtual bool + has_more_output(const encoder_state* state) const noexcept = 0;
| # Zlib | ||
| find_package(ZLIB) | ||
| if (ZLIB_FOUND) | ||
| file(GLOB_RECURSE BOOST_HTTP_ZLIB_HEADERS CONFIGURE_DEPENDS include/boost/http/zlib/*.hpp) | ||
| file(GLOB_RECURSE BOOST_HTTP_ZLIB_SOURCES CONFIGURE_DEPENDS src_zlib/*.cpp src_zlib/*.hpp) | ||
| source_group("" FILES "include/boost/http/zlib.hpp") | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/include/boost/http/zlib PREFIX "include" FILES ${BOOST_HTTP_ZLIB_HEADERS}) | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/src_zlib PREFIX "src" FILES ${BOOST_HTTP_ZLIB_SOURCES}) | ||
| add_library(boost_http_zlib include/boost/http/zlib.hpp build/Jamfile ${BOOST_HTTP_ZLIB_HEADERS} ${BOOST_HTTP_ZLIB_SOURCES}) | ||
| add_library(Boost::http_zlib ALIAS boost_http_zlib) | ||
| target_link_libraries(boost_http_zlib PUBLIC boost_http) | ||
| target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) | ||
| target_compile_definitions(boost_http_zlib PUBLIC BOOST_HTTP_HAS_ZLIB) | ||
| target_compile_definitions(boost_http_zlib PRIVATE BOOST_HTTP_SOURCE) | ||
| endif () | ||
|
|
||
| # Brotli | ||
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") | ||
| find_package(Brotli) | ||
| if (Brotli_FOUND) | ||
| file(GLOB_RECURSE BOOST_HTTP_BROTLI_HEADERS CONFIGURE_DEPENDS include/boost/http/brotli/*.hpp) | ||
| file(GLOB_RECURSE BOOST_HTTP_BROTLI_SOURCES CONFIGURE_DEPENDS src_brotli/*.cpp src_brotli/*.hpp) | ||
| source_group("" FILES "include/boost/http/brotli.hpp") | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/include/boost/http/brotli PREFIX "include" FILES ${BOOST_HTTP_BROTLI_HEADERS}) | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/src_brotli PREFIX "src" FILES ${BOOST_HTTP_BROTLI_SOURCES}) | ||
| add_library(boost_http_brotli include/boost/http/brotli.hpp build/Jamfile ${BOOST_HTTP_BROTLI_HEADERS} ${BOOST_HTTP_BROTLI_SOURCES}) | ||
| add_library(Boost::http_brotli ALIAS boost_http_brotli) | ||
| target_link_libraries(boost_http_brotli PUBLIC boost_http) | ||
| target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) | ||
| target_compile_definitions(boost_http_brotli PUBLIC BOOST_HTTP_HAS_BROTLI) | ||
| target_compile_definitions(boost_http_brotli PRIVATE BOOST_HTTP_SOURCE) | ||
| endif () |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CMakeLists.txt" -type f | head -20Repository: cppalliance/http
Length of output: 187
🏁 Script executed:
cat -n CMakeLists.txt | sed -n '183,214p'Repository: cppalliance/http
Length of output: 2407
🏁 Script executed:
# Check if boost_http_zlib/brotli are exported or installed
rg -n "boost_http_zlib|boost_http_brotli" CMakeLists.txt | head -30Repository: cppalliance/http
Length of output: 1079
🏁 Script executed:
# Check if these targets are exported or installed
rg -n "TARGETS.*boost_http_zlib|TARGETS.*boost_http_brotli|install" CMakeLists.txt | tail -40Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
wc -l CMakeLists.txtRepository: cppalliance/http
Length of output: 78
🏁 Script executed:
cat -n CMakeLists.txt | sed -n '215,232p'Repository: cppalliance/http
Length of output: 554
🏁 Script executed:
cat -n CMakeLists.txt | sed -n '1,100p'Repository: cppalliance/http
Length of output: 4000
🏁 Script executed:
cat -n CMakeLists.txt | sed -n '100,182p'Repository: cppalliance/http
Length of output: 4179
🏁 Script executed:
# Search for usage of boost_http_zlib and boost_http_brotli in tests
rg -r "boost_http_zlib|boost_http_brotli|Boost::http_zlib|Boost::http_brotli" test/Repository: cppalliance/http
Length of output: 223
🏁 Script executed:
# Check if these libraries are linked in test CMakeLists files
rg -n "boost_http_zlib|boost_http_brotli|Boost::http_zlib|Boost::http_brotli" test/CMakeLists.txt test/unit/CMakeLists.txt test/limits/CMakeLists.txtRepository: cppalliance/http
Length of output: 371
🏁 Script executed:
# Check if boost_http includes zlib/brotli headers directly
rg "include.*zlib|include.*brotli" include/boost/http.hpp include/boost/http/Repository: cppalliance/http
Length of output: 1825
🏁 Script executed:
# Check if boost_http.hpp or main boost_http headers include zlib/brotli
cat include/boost/http.hppRepository: cppalliance/http
Length of output: 1828
🏁 Script executed:
# Check if there are CMake config/package files that export these libraries
find . -name "*Config.cmake" -o -name "*config.cmake" -o -name "*.pc"Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check all source files to see if zlib/brotli are used in boost_http core
rg "ZLIB|Brotli|zlib|brotli" src/ include/boost/http/ --exclude="*zlib*" --exclude="*brotli*"Repository: cppalliance/http
Length of output: 471
🏁 Script executed:
# Check all source files to see if zlib/brotli are used in boost_http core
rg "ZLIB|Brotli" src/ include/boost/http/ -g "!*zlib*" -g "!*brotli*"Repository: cppalliance/http
Length of output: 623
🏁 Script executed:
# Check the library types (static vs shared) in CMakeLists.txt
rg -n "add_library" CMakeLists.txtRepository: cppalliance/http
Length of output: 778
Make compression library dependencies PUBLIC for proper static linking.
boost_http_zlib and boost_http_brotli link their external dependencies as PRIVATE. In static builds, when consumers link these wrapper libraries, they won't automatically receive the necessary linker flags for ZLIB or Brotli, potentially causing unresolved symbol errors if the implementation calls functions from these libraries.
Change to:
target_link_libraries(boost_http_zlib PUBLIC boost_http ZLIB::ZLIB)
target_link_libraries(boost_http_brotli PUBLIC boost_http Brotli::common Brotli::decoder Brotli::encoder)This ensures proper dependency propagation in static builds and matches CMake best practices for library packaging.
🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 183 - 214, The CMake rules currently link ZLIB
and Brotli as PRIVATE to boost_http_zlib and boost_http_brotli, which prevents
consumers from inheriting those linker dependencies; update the two
target_link_libraries calls so dependencies are PUBLIC: change
target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) to
target_link_libraries(boost_http_zlib PUBLIC ZLIB::ZLIB) and change
target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder
Brotli::encoder) to target_link_libraries(boost_http_brotli PUBLIC
Brotli::common Brotli::decoder Brotli::encoder) so that consumers of
Boost::http_zlib and Boost::http_brotli receive the necessary linker flags for
static linking.
| [source,cpp] | ||
| ---- | ||
| // Load custom dictionary | ||
| brotli::shared_dictionary dict = load_dictionary(); | ||
| // Install services with dictionary | ||
| auto& encoder = brotli::install_encode_service(ctx, dict); | ||
| auto& decoder = brotli::install_decode_service(ctx, dict); | ||
| ---- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for install_encode_service and install_decode_service function signatures
echo "=== Searching for install_encode_service signatures ==="
rg -n 'install_encode_service' --type cpp -B 2 -A 5
echo ""
echo "=== Searching for install_decode_service signatures ==="
rg -n 'install_decode_service' --type cpp -B 2 -A 5
echo ""
echo "=== Searching in header files ==="
rg -n 'install_encode_service|install_decode_service' --type h -B 2 -A 5Repository: cppalliance/http
Length of output: 9755
Remove incorrect dictionary parameters from the example code.
The functions install_encode_service() and install_decode_service() accept only the context parameter (capy::polystore& ctx). The example incorrectly shows passing a dictionary parameter. Update the code snippet to remove the dict arguments:
Corrected code:
[source,cpp]
----
// Install services
auto& encoder = brotli::install_encode_service(ctx);
auto& decoder = brotli::install_decode_service(ctx);
----🤖 Prompt for AI Agents
In `@doc/modules/ROOT/pages/compression/brotli.adoc` around lines 76 - 84, Update
the example to stop passing a dictionary to the service-install functions:
remove the second argument from brotli::install_encode_service and
brotli::install_decode_service so each is called with only the context parameter
(capy::polystore& ctx) as their signatures expect; ensure the load_dictionary
call can remain or be removed as appropriate, but do not pass dict into
install_encode_service or install_decode_service.
| @code | ||
| #include <boost/http/brotli.hpp> | ||
| #include <boost/capy/datastore.hpp> | ||
| // Create a datastore for services | ||
| boost::capy::datastore ctx; | ||
| // Install compression and decompression services | ||
| auto& encoder = boost::http::brotli::install_encode_service(ctx); | ||
| auto& decoder = boost::http::brotli::install_decode_service(ctx); | ||
| @endcode | ||
| */ |
There was a problem hiding this comment.
Documentation references incorrect type.
Same issue as in zlib.hpp — the example uses boost::capy::datastore but should use capy::polystore to match actual usage.
Suggested fix
`@code`
`#include` <boost/http/brotli.hpp>
- `#include` <boost/capy/datastore.hpp>
+ `#include` <boost/capy/core/polystore.hpp>
// Create a datastore for services
- boost::capy::datastore ctx;
+ boost::capy::polystore ctx;
// Install compression and decompression services
auto& encoder = boost::http::brotli::install_encode_service(ctx);
auto& decoder = boost::http::brotli::install_decode_service(ctx);
`@endcode`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @code | |
| #include <boost/http/brotli.hpp> | |
| #include <boost/capy/datastore.hpp> | |
| // Create a datastore for services | |
| boost::capy::datastore ctx; | |
| // Install compression and decompression services | |
| auto& encoder = boost::http::brotli::install_encode_service(ctx); | |
| auto& decoder = boost::http::brotli::install_decode_service(ctx); | |
| @endcode | |
| */ | |
| `@code` | |
| `#include` <boost/http/brotli.hpp> | |
| `#include` <boost/capy/core/polystore.hpp> | |
| // Create a datastore for services | |
| boost::capy::polystore ctx; | |
| // Install compression and decompression services | |
| auto& encoder = boost::http::brotli::install_encode_service(ctx); | |
| auto& decoder = boost::http::brotli::install_decode_service(ctx); | |
| `@endcode` |
🤖 Prompt for AI Agents
In `@include/boost/http/brotli.hpp` around lines 21 - 32, The code example in
include/boost/http/brotli.hpp uses the wrong type for the context; replace the
example's boost::capy::datastore with capy::polystore so it matches actual
usage; update the snippet that creates the context and the subsequent calls to
install_encode_service and install_decode_service (references:
install_encode_service, install_decode_service) to use capy::polystore for
correctness.
| @code | ||
| #include <boost/http/zlib.hpp> | ||
| #include <boost/capy/datastore.hpp> | ||
| // Create a datastore for services | ||
| boost::capy::datastore ctx; | ||
| // Install compression and decompression services | ||
| auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx); | ||
| auto& inflate_svc = boost::http::zlib::install_inflate_service(ctx); | ||
| @endcode |
There was a problem hiding this comment.
Documentation references incorrect type.
The example uses boost::capy::datastore but the actual type used throughout the codebase is capy::polystore (as seen in tests and implementation files).
Suggested fix
`@code`
`#include` <boost/http/zlib.hpp>
- `#include` <boost/capy/datastore.hpp>
+ `#include` <boost/capy/core/polystore.hpp>
// Create a datastore for services
- boost::capy::datastore ctx;
+ boost::capy::polystore ctx;
// Install compression and decompression services
auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx);
auto& inflate_svc = boost::http::zlib::install_inflate_service(ctx);
`@endcode`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @code | |
| #include <boost/http/zlib.hpp> | |
| #include <boost/capy/datastore.hpp> | |
| // Create a datastore for services | |
| boost::capy::datastore ctx; | |
| // Install compression and decompression services | |
| auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx); | |
| auto& inflate_svc = boost::http::zlib::install_inflate_service(ctx); | |
| @endcode | |
| `@code` | |
| `#include` <boost/http/zlib.hpp> | |
| `#include` <boost/capy/core/polystore.hpp> | |
| // Create a datastore for services | |
| boost::capy::polystore ctx; | |
| // Install compression and decompression services | |
| auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx); | |
| auto& inflate_svc = boost::http::zlib::install_inflate_service(ctx); | |
| `@endcode` |
🤖 Prompt for AI Agents
In `@include/boost/http/zlib.hpp` around lines 22 - 32, The doc example uses the
wrong type; replace the datastore usage with the actual project polystore type
by changing the include and variable type to capy::polystore and construct a
capy::polystore named (e.g.) ctx, then pass that to
boost::http::zlib::install_deflate_service and
boost::http::zlib::install_inflate_service (use the capy::polystore symbol
instead of boost::capy::datastore and update the include to the header that
provides capy::polystore).
| template<bool isLayoutIdentical = | ||
| is_layout_identical<stream, z_stream_s>()> | ||
| struct stream_cast_impl | ||
| { | ||
| explicit | ||
| stream_cast_impl( | ||
| stream& st) | ||
| : pzs_(reinterpret_cast<z_stream_s*>(&st)) | ||
| , st_(st) | ||
| { | ||
| zs_.next_in = st_.next_in; | ||
| zs_.avail_in = st_.avail_in; | ||
| zs_.total_in = st_.total_in; | ||
| zs_.next_out = st_.next_out; | ||
| zs_.avail_out = st_.avail_out; | ||
| zs_.total_out = st_.total_out; | ||
| zs_.msg = nullptr; | ||
| zs_.state = reinterpret_cast< | ||
| internal_state*>(st_.state); | ||
| zs_.zalloc = st_.zalloc; | ||
| zs_.zfree = st_.zfree; | ||
| zs_.opaque = st_.opaque; | ||
| zs_.data_type = st_.data_type; | ||
| zs_.adler = st_.adler; | ||
| zs_.reserved = st_.reserved; | ||
| } | ||
|
|
||
| ~stream_cast_impl() | ||
| { | ||
| st_.next_in = zs_.next_in; | ||
| st_.avail_in = zs_.avail_in; | ||
| st_.total_in = zs_.total_in; | ||
| st_.next_out = zs_.next_out; | ||
| st_.avail_out = zs_.avail_out; | ||
| st_.total_out = zs_.total_out; | ||
| st_.msg = zs_.msg; | ||
| st_.state = zs_.state; | ||
| st_.zalloc = zs_.zalloc; | ||
| st_.zfree = zs_.zfree; | ||
| st_.opaque = zs_.opaque; | ||
| st_.data_type = zs_.data_type; | ||
| st_.adler = zs_.adler; | ||
| st_.reserved = zs_.reserved; | ||
| } | ||
|
|
||
| z_stream_s* | ||
| get() noexcept | ||
| { | ||
| return pzs_; | ||
| } | ||
|
|
||
| private: | ||
| z_stream_s* pzs_ = nullptr; | ||
| stream& st_; | ||
| z_stream_s zs_; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "stream_cast.hpp" | head -20Repository: cppalliance/http
Length of output: 176
🏁 Script executed:
cat -n src_zlib/stream_cast.hpp | head -150Repository: cppalliance/http
Length of output: 4585
Fix fallback stream_cast_impl: pzs_ points at the wrong object.
When layouts are incompatible (primary template), pzs_ currently points to stream via reinterpret_cast, causing zlib to write to an object with mismatched field offsets. The destructor then overwrites the corrupted data with stale values, discarding all real updates. pzs_ should point to zs_ instead.
Proposed fix
explicit
stream_cast_impl(
stream& st)
- : pzs_(reinterpret_cast<z_stream_s*>(&st))
- , st_(st)
+ : st_(st)
{
zs_.next_in = st_.next_in;
zs_.avail_in = st_.avail_in;
zs_.total_in = st_.total_in;
@@
zs_.data_type = st_.data_type;
zs_.adler = st_.adler;
zs_.reserved = st_.reserved;
+
+ pzs_ = &zs_;
}🤖 Prompt for AI Agents
In `@src_zlib/stream_cast.hpp` around lines 56 - 111, The fallback
stream_cast_impl currently sets pzs_ to reinterpret_cast<z_stream_s*>(&st),
causing writes to the wrong object; update the constructor so pzs_ points to the
internal zs_ (i.e., initialize pzs_ to &zs_ instead of &st) and ensure get(),
destructor and member declarations continue to use pzs_, zs_ and st_ so zlib
writes go to zs_ and then the destructor copies those updated fields back into
stream st_; adjust any cast/initialization logic in stream_cast_impl's
constructor accordingly.
| void | ||
| test_error_code() | ||
| { | ||
| // TODO | ||
| boost::system::error_code ec{ brotli::error::no_error }; | ||
| } | ||
|
|
||
| void | ||
| test_decode() | ||
| { | ||
| // TODO | ||
| capy::polystore ctx; | ||
| brotli::install_decode_service(ctx); | ||
| } | ||
|
|
||
| void | ||
| test_encode() | ||
| { | ||
| // TODO | ||
| capy::polystore ctx; | ||
| brotli::install_encode_service(ctx); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "brotli.cpp" -type fRepository: cppalliance/http
Length of output: 82
🏁 Script executed:
sed -n '20,41p' ./test/unit/brotli.cppRepository: cppalliance/http
Length of output: 443
🏁 Script executed:
rg "install_decode_service|install_encode_service" --max-count 10 -B 2 -A 2Repository: cppalliance/http
Length of output: 4939
🏁 Script executed:
rg "class polystore" -B 2 -A 10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "polystore.*\.get|ctx\.get" test/ -B 1 -A 1Repository: cppalliance/http
Length of output: 850
🏁 Script executed:
head -30 ./test/unit/brotli.cppRepository: cppalliance/http
Length of output: 658
🏁 Script executed:
cat test/unit/test_helpers.hpp | head -50Repository: cppalliance/http
Length of output: 1210
🏁 Script executed:
cat test/unit/test_suite.hpp | head -40Repository: cppalliance/http
Length of output: 116
🏁 Script executed:
find . -name "test_suite.hpp" -type fRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "BOOST_TEST" test/unit/brotli.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "BOOST_TEST|BOOST_CHECK|TEST\(|test\(" test/unit/ -m 3 | head -30Repository: cppalliance/http
Length of output: 1930
🏁 Script executed:
cat ./test/unit/brotli.cppRepository: cppalliance/http
Length of output: 1109
Add real assertions to the Brotli smoke tests (currently TODO placeholders).
Lines 23, 30, and 38 contain TODOs, so these tests pass without validating anything. Consider asserting the error_code and that the installed services are retrievable from the polystore.
✅ Suggested assertions
void
test_error_code()
{
- // TODO
boost::system::error_code ec{ brotli::error::no_error };
+ BOOST_TEST(ec == brotli::error::no_error);
}
void
test_decode()
{
- // TODO
capy::polystore ctx;
- brotli::install_decode_service(ctx);
+ auto& svc = brotli::install_decode_service(ctx);
+ BOOST_TEST(&svc == &ctx.get<brotli::decode_service>());
}
void
test_encode()
{
- // TODO
capy::polystore ctx;
- brotli::install_encode_service(ctx);
+ auto& svc = brotli::install_encode_service(ctx);
+ BOOST_TEST(&svc == &ctx.get<brotli::encode_service>());
}If you want, I can extend this into an encode/decode round‑trip test.
🤖 Prompt for AI Agents
In `@test/unit/brotli.cpp` around lines 20 - 41, Replace the TODO placeholders
with real assertions: in test_error_code assert that boost::system::error_code
ec constructed with brotli::error::no_error compares equal to
brotli::error::no_error (or ec.value()==0); in test_decode after creating
capy::polystore ctx and calling brotli::install_decode_service(ctx) assert that
the decode service can be retrieved from ctx (e.g., ctx.get_service<...>() is
non-null/has expected type); similarly in test_encode assert that after
brotli::install_encode_service(ctx) the encode service is retrievable from ctx.
Reference test functions test_error_code, test_decode, test_encode and symbols
brotli::error::no_error, capy::polystore, brotli::install_decode_service,
brotli::install_encode_service.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@doc/modules/ROOT/pages/compression/zlib.adoc`:
- Around line 1-149: The docs use non-existent types and wrong API names; update
examples to the actual stream-based interface: replace references to
deflate_result/inflate_result with int return values, remove simple buffer
overloads and instead show creating a zlib::stream, setting
next_in/avail_in/next_out/avail_out, calling deflate_svc.init(stream&, level) or
init2(...) then deflate_svc.deflate(stream&, zlib::flush::...), and
deflate_svc.deflate_end(); for inflate show inflate_svc.init/init2,
inflate_svc.inflate(stream&, ...), and inflate_end(); rename enum constants to
default_compression/no_compression/best_speed/best_compression and
default_strategy/huffman_only where used; replace deflateBound() with
deflate_service::bound(stream&, unsigned long) and demonstrate using bound(...)
for output sizing.
In `@include/boost/http/brotli/encode.hpp`:
- Around line 13-17: The header uses std::size_t but only includes <cstdint>, so
make the header self-contained by adding the missing include for <cstddef>;
update include list in encode.hpp (near boost/http/detail/config.hpp and
boost/http/brotli/shared_dictionary.hpp) to also include <cstddef> so all uses
of std::size_t in functions/types within this header are defined.
- Around line 155-158: The example uses boost::capy::datastore but the
install_encode_service signature expects a capy::polystore; update the example
to construct a boost::capy::polystore (e.g., replace datastore with polystore)
before calling boost::http::brotli::install_encode_service(ctx) so the types
match the function signature and tests.
In `@include/boost/http/zlib/deflate.hpp`:
- Around line 39-41: The example incorrectly declares boost::capy::datastore;
replace it with the correct type boost::capy::polystore (or capy::polystore) so
the call to boost::http::zlib::install_deflate_service(ctx) receives the
expected capy::polystore&. Update the variable declaration and any related
references in the snippet to use polystore to match tests and documentation.
In `@include/boost/http/zlib/impl/error.hpp`:
- Around line 14-18: The headers that use std::underlying_type (the error.hpp
variants that define the error enum -> error_code conversion) are missing an
explicit `#include` <type_traits>; add `#include` <type_traits> near the top of each
of those error.hpp headers so std::underlying_type is provided directly (fix the
files that use std::underlying_type in their enum-to-error_code code paths).
♻️ Duplicate comments (5)
include/boost/http/brotli/error.hpp (1)
36-41: Fix truncated Brotli enum identifiers to match upstream names.
Two identifiers appear truncated and should align with BrotliDecoderErrorCode constants (e.g., *_nibble, *_repeat).🔧 Proposed fix
- format_exuberant_meta_nibbl = -3, + format_exuberant_meta_nibble = -3, ... - format_context_map_repea = -8, + format_context_map_repeat = -8,CMakeLists.txt (1)
191-212: Expose ZLIB/Brotli link dependencies to consumers.
boost_http_zlibandboost_http_brotlistill link external libs asPRIVATE, which can break static linking for downstream users.🛠️ Proposed fix
-target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) +target_link_libraries(boost_http_zlib PUBLIC ZLIB::ZLIB) ... -target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) +target_link_libraries(boost_http_brotli PUBLIC Brotli::common Brotli::decoder Brotli::encoder)src_zlib/stream_cast.hpp (1)
60-65: Fix fallback stream_cast to avoid writing into mismatched layout.When
isLayoutIdenticalis false, Line 63 pointspzs_atstream, so zlib writes into a differently laid-out object and updates are lost when the destructor copies fromzs_.🛠️ Proposed fix
- stream_cast_impl( - stream& st) - : pzs_(reinterpret_cast<z_stream_s*>(&st)) - , st_(st) + stream_cast_impl( + stream& st) + : st_(st) { zs_.next_in = st_.next_in; zs_.avail_in = st_.avail_in; @@ zs_.adler = st_.adler; zs_.reserved = st_.reserved; + + pzs_ = &zs_; }include/boost/http/zlib/inflate.hpp (1)
35-39: Docs: usecapy::polystorein the example.Line 37 uses
boost::capy::datastore, butinstall_inflate_servicetakescapy::polystore&.🛠️ Proposed fix
- boost::capy::datastore ctx; + boost::capy::polystore ctx;src/serializer.cpp (1)
17-32: Feature-guard optional Brotli/Zlib includes (duplicate).This repeats the prior finding: optional Brotli/Zlib headers are still included unconditionally and will break builds when the deps are unavailable.
🧹 Nitpick comments (2)
include/boost/http/zlib/compression_strategy.hpp (1)
45-61: Enum values correctly match zlib constants.The values (0–4) align with zlib's
Z_DEFAULT_STRATEGY,Z_FILTERED,Z_HUFFMAN_ONLY,Z_RLE, andZ_FIXEDconstants, ensuring compatibility with the underlying zlib library.Nit: For explicit ABI compatibility with zlib's
inttype, you could consider specifying the underlying type:💡 Optional: Specify underlying type
-enum compression_strategy +enum compression_strategy : intinclude/boost/http/zlib/deflate.hpp (1)
179-184: Prefer a typed gzip header pointer overvoid*.Line 179’s
set_header(stream&, void* header)loses type safety and discoverability. If a gzip header type (e.g.,zlib::gz_headeror a wrapper inzlib/stream.hpp) exists, consider exposing that type here instead.
| #include <boost/http/detail/config.hpp> | ||
| #include <boost/http/brotli/shared_dictionary.hpp> | ||
|
|
||
| #include <cstdint> | ||
|
|
There was a problem hiding this comment.
Add <cstddef> for std::size_t.
std::size_t is used throughout but not guaranteed by <cstdint>. Add <cstddef> to make the header self-contained.
✅ Suggested fix
`#include` <boost/http/detail/config.hpp>
`#include` <boost/http/brotli/shared_dictionary.hpp>
+#include <cstddef>
`#include` <cstdint>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <boost/http/detail/config.hpp> | |
| #include <boost/http/brotli/shared_dictionary.hpp> | |
| #include <cstdint> | |
| `#include` <boost/http/detail/config.hpp> | |
| `#include` <boost/http/brotli/shared_dictionary.hpp> | |
| `#include` <cstddef> | |
| `#include` <cstdint> | |
🤖 Prompt for AI Agents
In `@include/boost/http/brotli/encode.hpp` around lines 13 - 17, The header uses
std::size_t but only includes <cstdint>, so make the header self-contained by
adding the missing include for <cstddef>; update include list in encode.hpp
(near boost/http/detail/config.hpp and boost/http/brotli/shared_dictionary.hpp)
to also include <cstddef> so all uses of std::size_t in functions/types within
this header are defined.
| // Example: Simple one-shot compression | ||
| boost::capy::datastore ctx; | ||
| auto& encoder = boost::http::brotli::install_encode_service(ctx); | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n include/boost/http/brotli/encode.hpp | sed -n '150,165p'Repository: cppalliance/http
Length of output: 805
🏁 Script executed:
rg "install_encode_service" --context 5 -A 10Repository: cppalliance/http
Length of output: 6225
🏁 Script executed:
rg "datastore|polystore" include/boost/http/brotli/encode.hpp -B 2 -A 2Repository: cppalliance/http
Length of output: 481
🏁 Script executed:
rg "class datastore|struct datastore" -A 5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "class polystore|struct polystore" -A 5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "using datastore" -B 2 -A 2Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
fd "polystore\|datastore" --type fRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "datastore|polystore" --type hpp | head -30Repository: cppalliance/http
Length of output: 87
🏁 Script executed:
rg "install_encode_service" -B 5 test/Repository: cppalliance/http
Length of output: 1100
🏁 Script executed:
rg "boost::capy::" -A 2 -B 2Repository: cppalliance/http
Length of output: 2786
🏁 Script executed:
fd "capy" --type fRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "install_deflate_service|install_inflate_service" -A 2Repository: cppalliance/http
Length of output: 3247
🏁 Script executed:
cat -n include/boost/http/zlib/deflate.hpp | sed -n '120,140p'Repository: cppalliance/http
Length of output: 1004
🏁 Script executed:
rg "install_deflate_service" include/boost/http/zlib/deflate.hpp -B 5 -A 2Repository: cppalliance/http
Length of output: 495
🏁 Script executed:
rg "class zlib" include/boost/http/ -A 10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "class.*datastore|struct.*datastore" -A 10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "namespace capy" -A 30Repository: cppalliance/http
Length of output: 42
Update example to use polystore instead of datastore.
The function signature requires capy::polystore&, but the example uses boost::capy::datastore. This is inconsistent with the actual tests and official documentation, which both use polystore. Update the example:
Fix
- boost::capy::datastore ctx;
+ boost::capy::polystore ctx;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Example: Simple one-shot compression | |
| boost::capy::datastore ctx; | |
| auto& encoder = boost::http::brotli::install_encode_service(ctx); | |
| // Example: Simple one-shot compression | |
| boost::capy::polystore ctx; | |
| auto& encoder = boost::http::brotli::install_encode_service(ctx); |
🤖 Prompt for AI Agents
In `@include/boost/http/brotli/encode.hpp` around lines 155 - 158, The example
uses boost::capy::datastore but the install_encode_service signature expects a
capy::polystore; update the example to construct a boost::capy::polystore (e.g.,
replace datastore with polystore) before calling
boost::http::brotli::install_encode_service(ctx) so the types match the function
signature and tests.
| boost::capy::datastore ctx; | ||
| auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx); | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n include/boost/http/zlib/deflate.hpp | head -60Repository: cppalliance/http
Length of output: 2312
🏁 Script executed:
rg "boost::capy::datastore|capy::datastore" -t cpp -t hpp --max-count 20Repository: cppalliance/http
Length of output: 87
🏁 Script executed:
rg "boost::capy::polystore|capy::polystore" -t cpp -t hpp --max-count 20Repository: cppalliance/http
Length of output: 87
🏁 Script executed:
rg "polystore|datastore" -t cpp -t hpp | grep -E "(class|struct|using|typedef)" | head -20Repository: cppalliance/http
Length of output: 87
🏁 Script executed:
rg "datastore|polystore" --max-count 30Repository: cppalliance/http
Length of output: 9059
🏁 Script executed:
fd polystore_fwd.hppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg "class polystore|struct polystore" -A 3Repository: cppalliance/http
Length of output: 42
Doc example uses undefined boost::capy::datastore; should be polystore.
Line 39 references boost::capy::datastore, but this type does not exist in the codebase. The install_deflate_service() function (line 40) expects a capy::polystore& parameter, and all test code and official documentation consistently use polystore. The example will mislead or fail to compile for users.
📝 Proposed fix
- boost::capy::datastore ctx;
+ boost::capy::polystore ctx;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| boost::capy::datastore ctx; | |
| auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx); | |
| boost::capy::polystore ctx; | |
| auto& deflate_svc = boost::http::zlib::install_deflate_service(ctx); |
🤖 Prompt for AI Agents
In `@include/boost/http/zlib/deflate.hpp` around lines 39 - 41, The example
incorrectly declares boost::capy::datastore; replace it with the correct type
boost::capy::polystore (or capy::polystore) so the call to
boost::http::zlib::install_deflate_service(ctx) receives the expected
capy::polystore&. Update the variable declaration and any related references in
the snippet to use polystore to match tests and documentation.
| #include <boost/http/detail/config.hpp> | ||
|
|
||
| #include <boost/system/error_category.hpp> | ||
| #include <boost/system/is_error_code_enum.hpp> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file
find . -type f -name "error.hpp" | grep -E "zlib/impl"Repository: cppalliance/http
Length of output: 100
🏁 Script executed:
# Read the file to see its contents
cat -n include/boost/http/zlib/impl/error.hppRepository: cppalliance/http
Length of output: 2177
🏁 Script executed:
# Search for other uses of std::underlying_type in the codebase
rg "std::underlying_type" --type cpp --type hppRepository: cppalliance/http
Length of output: 87
🏁 Script executed:
# Check if any other files in boost/http include <type_traits>
fd -e hpp -e cpp | xargs grep -l "#include <type_traits>" | head -20Repository: cppalliance/http
Length of output: 434
🏁 Script executed:
# Correct search for std::underlying_type
rg "std::underlying_type"Repository: cppalliance/http
Length of output: 1143
🏁 Script executed:
# Check if boost/system/error_category.hpp includes <type_traits>
# First, let's see if boost/system headers are in this repo
find . -path "*/boost/system/*" -name "*.hpp" | head -10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if similar error impl files include <type_traits>
head -30 include/boost/http/impl/error.hppRepository: cppalliance/http
Length of output: 790
🏁 Script executed:
# Check brotli impl error header
head -30 include/boost/http/brotli/impl/error.hppRepository: cppalliance/http
Length of output: 779
🏁 Script executed:
# Check router_types.hpp
head -30 include/boost/http/server/router_types.hppRepository: cppalliance/http
Length of output: 1053
🏁 Script executed:
# Check full impl/error.hpp for <type_traits>
grep -n "type_traits" include/boost/http/impl/error.hppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check bcrypt/error.hpp
cat -n include/boost/http/bcrypt/error.hpp | head -40Repository: cppalliance/http
Length of output: 1189
🏁 Script executed:
# Also check if it has type_traits
grep -n "type_traits" include/boost/http/bcrypt/error.hppRepository: cppalliance/http
Length of output: 42
Add <type_traits> for std::underlying_type.
Lines 66–67 use std::underlying_type, but this header doesn't include <type_traits>. While compilation may succeed via transitive includes, this is brittle and inconsistent with similar code in the codebase (e.g., include/boost/http/server/router_types.hpp explicitly includes <type_traits>). Direct inclusion is the best practice for headers that use standard library types.
Proposed fix
`#include` <boost/system/error_category.hpp>
`#include` <boost/system/is_error_code_enum.hpp>
+#include <type_traits>Also applies to: include/boost/http/brotli/impl/error.hpp, include/boost/http/impl/error.hpp, include/boost/http/bcrypt/error.hpp
🤖 Prompt for AI Agents
In `@include/boost/http/zlib/impl/error.hpp` around lines 14 - 18, The headers
that use std::underlying_type (the error.hpp variants that define the error enum
-> error_code conversion) are missing an explicit `#include` <type_traits>; add
`#include` <type_traits> near the top of each of those error.hpp headers so
std::underlying_type is provided directly (fix the files that use
std::underlying_type in their enum-to-error_code code paths).
a0a3ffa to
6ff59bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/http/parser.hpp (1)
671-689: Update deflate/gzip docs to reference the correct service namespace.The documentation for
apply_deflate_decoderandapply_gzip_decoderincorrectly referencesboost::zlib::inflate_service. They should referenceboost::http::zlib::inflate_serviceto match the actual API namespace and be consistent with theboost::http::brotli::decode_servicedocumented for brotli.📝 Proposed doc fix
- Requires `boost::zlib::inflate_service` to be + Requires `boost::http::zlib::inflate_service` to be installed, otherwise an exception is thrown.Apply this change to both the deflate and gzip documentation comments.
🤖 Fix all issues with AI agents
In `@build/brotli.jam`:
- Around line 55-101: The init rule documents falling back to
BROTLI_LIBRARY_PATH, BROTLI_NAME, and BROTLI_INCLUDE when no options are
provided but currently only reads options via feature.get-values and never
checks environment variables; update init to detect when !$(options) (or
is-default true) and read those env vars (use getenv for BROTLI_LIBRARY_PATH,
BROTLI_NAME, BROTLI_INCLUDE), populate library-path/include-path/brotli*-name
from the env values before calling ac-library, and preserve existing behavior
when explicit options exist (refer to the init rule, $(options),
feature.get-values, library-path, include-path,
brotlicommon-name/brotlidec-name/brotlienc-name, and is-default).
In `@include/boost/http/brotli/decode.hpp`:
- Around line 13-18: This header uses alloc_func, free_func and std::size_t but
doesn't guarantee their declarations; make decode.hpp self-contained by adding
an include for <cstddef> to provide std::size_t and also include (or otherwise
provide) the brotli types header that defines alloc_func and free_func (the
brotli types.hpp) so decode.hpp no longer relies on include order or implicit
transitive includes.
In `@include/boost/http/brotli/shared_dictionary.hpp`:
- Around line 43-63: shared_dictionary_service currently has its virtual
interface disabled by the `#if 0` guard, which breaks the overrides in
src_brotli/shared_dictionary.cpp (create_instance, destroy_instance, attach) and
makes install_shared_dictionary_service unusable; remove the `#if 0`/`#endif` so
the virtual methods (create_instance, destroy_instance, attach) are declared,
and add an `#include <cstdint>` to this header so `uint8_t` is defined, or
alternatively keep the guard and remove/stop using the override methods in the
implementation to match the stubbed interface.
♻️ Duplicate comments (9)
include/boost/http/zlib/impl/error.hpp (1)
14-18: Add<type_traits>forstd::underlying_type.
This header usesstd::underlying_type(Lines 66–67) but doesn’t include<type_traits>. Add it to keep the header self-contained.Proposed fix
`#include` <boost/system/error_category.hpp> `#include` <boost/system/is_error_code_enum.hpp> +#include <type_traits>CMakeLists.txt (1)
193-211: Propagate Zlib/Brotli link deps as PUBLIC for static consumers.
boost_http_zlibandboost_http_brotlilink ZLIB/Brotli asPRIVATE, which can break static consumers that only linkBoost::http_zlib/Boost::http_brotli. Make these dependenciesPUBLIC.Proposed fix
- target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) + target_link_libraries(boost_http_zlib PUBLIC ZLIB::ZLIB) ... - target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) + target_link_libraries(boost_http_brotli PUBLIC Brotli::common Brotli::decoder Brotli::encoder)include/boost/http/brotli/encode.hpp (2)
13-16: Add<cstddef>sostd::size_tis guaranteed here.
std::size_tis used throughout this header, but<cstdint>doesn’t guarantee it. Add<cstddef>to keep the header self-contained.🩹 Proposed fix
`#include` <boost/http/detail/config.hpp> `#include` <boost/http/brotli/shared_dictionary.hpp> +#include <cstddef> `#include` <cstdint>
154-158: Usecapy::polystorein the example to match the API.
install_encode_servicetakescapy::polystore&, so the sample as written won’t compile.🩹 Proposed fix
- boost::capy::datastore ctx; + boost::capy::polystore ctx;include/boost/http/zlib/inflate.hpp (1)
35-39: Usecapy::polystorein the example to match the API.
install_inflate_serviceexpectscapy::polystore&, so the current example is type-inconsistent.🩹 Proposed fix
- boost::capy::datastore ctx; + boost::capy::polystore ctx;doc/modules/ROOT/pages/compression/zlib.adoc (1)
32-164: Docs still describe non-existent buffer APIs and types.The headers expose stream-based
deflate_service/inflate_servicereturningint, but these examples referencedeflateBound,deflate_result/inflate_result, and enum names that don’t exist in the new public API. Please realign the code snippets and error handling to the actual stream interface (zlib::stream,init/init2,deflate/inflate,deflate_end/inflate_end) and the real enum constants from the headers.include/boost/http/brotli.hpp (1)
21-30: Fix example to usecapy::polystore(notdatastore).The service install APIs take a
capy::polystore&, so the example should reflect that to avoid misleading users.📝 Suggested doc fix
- `#include` <boost/capy/datastore.hpp> + `#include` <boost/capy/core/polystore.hpp> - // Create a datastore for services - boost::capy::datastore ctx; + // Create a polystore for services + boost::capy::polystore ctx;include/boost/http/brotli/error.hpp (1)
33-42: Fix truncated enum identifiers to match Brotli upstream.These two names appear truncated and should match the official Brotli error constants.
📝 Suggested fix
- format_exuberant_meta_nibbl = -3, + format_exuberant_meta_nibble = -3, ... - format_context_map_repea = -8, + format_context_map_repeat = -8,build/Jamfile (1)
63-92: Propagate zlib/brotli link libs to dependents for static builds.
External compression libs are only inrequirements, so dependents can miss link libraries in static builds. Add them tousage-requirementsto propagate transitively.🛠️ Proposed Jamfile adjustment
lib boost_http_zlib : http_zlib_sources : requirements <library>/boost/http//boost_http [ ac.check-library /zlib//zlib : <library>/zlib//zlib : <build>no ] : usage-requirements <library>/boost/http//boost_http <define>BOOST_HTTP_HAS_ZLIB + <library>/zlib//zlib ; @@ lib boost_http_brotli : http_brotli_sources : requirements <library>/boost/http//boost_http [ ac.check-library /brotli//brotlicommon : <library>/brotli//brotlicommon : <build>no ] [ ac.check-library /brotli//brotlidec : <library>/brotli//brotlidec : <build>no ] [ ac.check-library /brotli//brotlienc : <library>/brotli//brotlienc : <build>no ] : usage-requirements <library>/boost/http//boost_http <define>BOOST_HTTP_HAS_BROTLI + <library>/brotli//brotlicommon + <library>/brotli//brotlidec + <library>/brotli//brotlienc ;Please verify with a
link=staticbuild that the final link line includes the zlib/brotli libs for dependents.
🧹 Nitpick comments (5)
test/limits/CMakeLists.txt (1)
25-33: Consider guardingBoost::corosiolinkage for clarity, though configure won't fail.While the root CMakeLists.txt creates an alias target (
Boost::headers) for missing Boost libraries via the conditional at lines 127–129, this implicit fallback makes the dependency less obvious. Using an explicitif(TARGET)guard would clarify the conditional nature and improve maintainability.🔧 Suggested CMake adjustment
target_link_libraries(boost_http_limits PRIVATE Boost::align Boost::assert Boost::config Boost::container_hash Boost::capy - Boost::corosio Boost::system Boost::throw_exception Boost::url Boost::utility) + +if (TARGET Boost::corosio) + target_link_libraries(boost_http_limits PRIVATE Boost::corosio) +endif()doc/modules/ROOT/pages/parsing.adoc (1)
268-281: Consider showing brotli decode-service installation alongside the config.The snippet enables
apply_brotli_decoder, but readers may miss that the brotli decode service must be installed in the same context. A one-liner or short note here would make the example more immediately runnable.test/unit/zlib.cpp (1)
21-41: Consider turning the TODOs into real assertions.Right now the tests only instantiate objects without verifying behavior. Adding a minimal round‑trip or explicit assertions would make the suite more meaningful.
src_brotli/decode.cpp (1)
149-177: Consider adding a TODO comment explaining why this code is disabled.The
#if 0block contains dictionary and metadata callback stubs. A brief comment explaining when/why this will be enabled would help future maintainers understand the roadmap.src_brotli/encode.cpp (1)
139-196: Consider adding a TODO comment for the disabled dictionary functionality.Similar to decode.cpp, documenting why this block is disabled and when it's planned for activation would improve maintainability. This block contains more extensive functionality (prepared dictionaries, memory estimation) than the decode counterpart.
| # If none of these options is specified, then the environmental | ||
| # variables BROTLI_LIBRARY_PATH, BROTLI_NAME, and BROTLI_INCLUDE will | ||
| # be used instead. | ||
| # | ||
| # Examples:: | ||
| # | ||
| # # Find brotli in the default system location | ||
| # using brotli ; | ||
| # # Find brotli in /usr/local | ||
| # using brotli : 1.1.0 | ||
| # : <include>/usr/local/include <search>/usr/local/lib ; | ||
| # | ||
| rule init ( | ||
| version ? | ||
| # (currently ignored) | ||
|
|
||
| : options * | ||
| # A list of the options to use | ||
|
|
||
| : requirements * | ||
| # The requirements for the target | ||
|
|
||
| : is-default ? | ||
| ) | ||
| { | ||
| local caller = [ project.current ] ; | ||
|
|
||
| if ! $(.initialized) | ||
| { | ||
| .initialized = true ; | ||
|
|
||
| project.initialize $(__name__) ; | ||
| .project = [ project.current ] ; | ||
| project brotli ; | ||
| } | ||
|
|
||
| local library-path = [ feature.get-values <search> : $(options) ] ; | ||
| local include-path = [ feature.get-values <include> : $(options) ] ; | ||
| local brotlicommon-name = [ feature.get-values <brotlicommon-name> : $(options) ] ; | ||
| local brotlidec-name = [ feature.get-values <brotlidec-name> : $(options) ] ; | ||
| local brotlienc-name = [ feature.get-values <brotlienc-name> : $(options) ] ; | ||
| local dll-paths = [ property.select <dll-path> : $(options) ] ; | ||
|
|
||
| if ! $(options) | ||
| { | ||
| is-default = true ; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the full brotli.jam file to see the complete init rule
cd build && wc -l brotli.jam && head -150 brotli.jamRepository: cppalliance/http
Length of output: 4601
🏁 Script executed:
# Search for BROTLI_LIBRARY_PATH, BROTLI_NAME, and BROTLI_INCLUDE across the codebase
rg "BROTLI_LIBRARY_PATH|BROTLI_NAME|BROTLI_INCLUDE" --type-listRepository: cppalliance/http
Length of output: 449
🏁 Script executed:
# Search for environment variable access patterns in the brotli.jam file
rg "getenv|ENV|BROTLI_" build/brotli.jamRepository: cppalliance/http
Length of output: 129
🏁 Script executed:
# Search for any getenv or environment variable reading in the codebase
rg "getenv" build/ --type jamRepository: cppalliance/http
Length of output: 87
🏁 Script executed:
# Check the ac module to see if it reads BROTLI_* env vars
fd "ac.jam" build/ --exec cat {}Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# More comprehensive search for BROTLI_LIBRARY_PATH usage anywhere
rg "BROTLI_LIBRARY_PATH" . --type-list=all 2>/dev/null || rg "BROTLI_LIBRARY_PATH"Repository: cppalliance/http
Length of output: 146
🏁 Script executed:
# Find the ac module
fd "ac" build/ -type fRepository: cppalliance/http
Length of output: 230
🏁 Script executed:
# Search for getenv or environment variable patterns in all jam files
rg "getenv|os.environ|BROTLI_" build/ -A 2 -B 2Repository: cppalliance/http
Length of output: 301
🏁 Script executed:
# Check for imports of ac in brotli.jam and look for how options are processed
cat build/brotli.jam | grep -A 50 "^import ac"Repository: cppalliance/http
Length of output: 1431
🏁 Script executed:
# Find where ac.jam or ac module is defined
find . -name "ac.jam" -o -name "ac.cpp" -o -name "ac*" | grep -E "ac\.(jam|cpp)" | head -20Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look in the entire repository for ac-library definition
rg "class ac-library|rule ac-library" -A 10Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if there are any other jam files that might define ac
ls -la build/Repository: cppalliance/http
Length of output: 292
🏁 Script executed:
# Read the complete brotli.jam to see full implementation
tail -100 build/brotli.jamRepository: cppalliance/http
Length of output: 3183
🏁 Script executed:
# Search for any getenv or module call that might read environment variables
rg "getenv|modules\.peek|module\.get" .Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if ac-library handles environment variables by searching in Boost.Build docs or similar modules
rg "LIBRARY_PATH|NAME|INCLUDE" build/ -B 3 -A 3Repository: cppalliance/http
Length of output: 398
🏁 Script executed:
# Check if there are similar library jam files in the repo to see if they follow the same pattern
find . -name "*.jam" -type f | head -10Repository: cppalliance/http
Length of output: 78
🏁 Script executed:
# Look for other library integration files to see if they use environment variable fallback
rg "environmental|LIBRARY_PATH|getenv" . --include="*.jam" -B 2 -A 2Repository: cppalliance/http
Length of output: 501
🏁 Script executed:
# Check if ac-library is documented anywhere in this repo
rg "ac-library|ac\.jam" .Repository: cppalliance/http
Length of output: 374
🏁 Script executed:
# Search web for Boost.Build ac-library documentation to understand if it handles env var fallback
curl -s "https://boostorg.github.io/build/" 2>/dev/null | grep -i "ac-library\|environment" || echo "Web request may not be available"Repository: cppalliance/http
Length of output: 92
🏁 Script executed:
# Check the git history to see when this comment was added and what was the intent
git log --oneline build/brotli.jam 2>/dev/null | head -20Repository: cppalliance/http
Length of output: 118
🏁 Script executed:
# Look for similar library initialization patterns in Boost.Build to understand the convention
rg "new ac-library" . -B 5 -A 5Repository: cppalliance/http
Length of output: 1583
🏁 Script executed:
# Look for any git commit details about when this was added to understand the original intent
git show 6ff59bb:build/brotli.jam 2>/dev/null | head -80Repository: cppalliance/http
Length of output: 2145
🏁 Script executed:
# Check if there are other similar modules in the wider Boost.Build ecosystem or documentation references
rg "ac-library" . -lRepository: cppalliance/http
Length of output: 78
🏁 Script executed:
# Search for any fallback logic when options are empty
grep -n "if ! \$(options)" build/brotli.jam -A 20Repository: cppalliance/http
Length of output: 631
Environment variable fallback documented but not implemented in init rule.
Lines 55–57 claim that BROTLI_LIBRARY_PATH, BROTLI_NAME, and BROTLI_INCLUDE environment variables will be used when no options are provided. However, the init rule only reads options from the $(options) parameter via feature.get-values and never accesses these environment variables. When !(options) is true, it merely sets is-default = true and passes empty paths to ac-library. Either implement the documented env var fallback using getenv or update the documentation to remove the misleading claim.
🤖 Prompt for AI Agents
In `@build/brotli.jam` around lines 55 - 101, The init rule documents falling back
to BROTLI_LIBRARY_PATH, BROTLI_NAME, and BROTLI_INCLUDE when no options are
provided but currently only reads options via feature.get-values and never
checks environment variables; update init to detect when !$(options) (or
is-default true) and read those env vars (use getenv for BROTLI_LIBRARY_PATH,
BROTLI_NAME, BROTLI_INCLUDE), populate library-path/include-path/brotli*-name
from the env values before calling ac-library, and preserve existing behavior
when explicit options exist (refer to the init rule, $(options),
feature.get-values, library-path, include-path,
brotlicommon-name/brotlidec-name/brotlienc-name, and is-default).
| #include <boost/http/detail/config.hpp> | ||
| #include <boost/http/brotli/error.hpp> | ||
| #include <boost/http/brotli/shared_dictionary.hpp> | ||
|
|
||
| #include <cstdint> | ||
|
|
There was a problem hiding this comment.
Ensure header self-containment for alloc/free and size_t.
alloc_func, free_func, and std::size_t are used but types.hpp / <cstddef> aren’t explicitly included here. If shared_dictionary.hpp doesn’t guarantee those, this header becomes order-dependent.
Proposed fix
`#include` <boost/http/detail/config.hpp>
`#include` <boost/http/brotli/error.hpp>
`#include` <boost/http/brotli/shared_dictionary.hpp>
+#include <boost/http/brotli/types.hpp>
`#include` <cstdint>
+#include <cstddef>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <boost/http/detail/config.hpp> | |
| #include <boost/http/brotli/error.hpp> | |
| #include <boost/http/brotli/shared_dictionary.hpp> | |
| #include <cstdint> | |
| `#include` <boost/http/detail/config.hpp> | |
| `#include` <boost/http/brotli/error.hpp> | |
| `#include` <boost/http/brotli/shared_dictionary.hpp> | |
| `#include` <boost/http/brotli/types.hpp> | |
| `#include` <cstdint> | |
| `#include` <cstddef> | |
🤖 Prompt for AI Agents
In `@include/boost/http/brotli/decode.hpp` around lines 13 - 18, This header uses
alloc_func, free_func and std::size_t but doesn't guarantee their declarations;
make decode.hpp self-contained by adding an include for <cstddef> to provide
std::size_t and also include (or otherwise provide) the brotli types header that
defines alloc_func and free_func (the brotli types.hpp) so decode.hpp no longer
relies on include order or implicit transitive includes.
| struct BOOST_SYMBOL_VISIBLE | ||
| shared_dictionary_service | ||
| { | ||
| #if 0 | ||
| virtual shared_dictionary* | ||
| create_instance( | ||
| alloc_func alloc_func, | ||
| free_func free_func, | ||
| void* opaque) const noexcept = 0; | ||
|
|
||
| virtual void | ||
| destroy_instance( | ||
| shared_dictionary* dict) const noexcept = 0; | ||
|
|
||
| virtual bool | ||
| attach( | ||
| shared_dictionary* dict, | ||
| shared_dictionary_type type, | ||
| std::size_t data_size, | ||
| const uint8_t data[]) const noexcept = 0; | ||
| #endif |
There was a problem hiding this comment.
Service interface is compiled out, breaking overrides.
With #if 0, shared_dictionary_service has no virtuals, but src_brotli/shared_dictionary.cpp defines create_instance/destroy_instance/attach with override, which will not compile and leaves install_shared_dictionary_service unusable. Please remove the guard (and include <cstdint> for uint8_t) or drop the overrides if this is intentionally stubbed.
🛠️ Proposed fix
`#include` <boost/http/detail/config.hpp>
`#include` <boost/http/brotli/types.hpp>
`#include` <boost/capy/core/polystore_fwd.hpp>
+#include <cstdint>
@@
struct BOOST_SYMBOL_VISIBLE
shared_dictionary_service
{
-#if 0
virtual shared_dictionary*
create_instance(
alloc_func alloc_func,
free_func free_func,
void* opaque) const noexcept = 0;
@@
virtual bool
attach(
shared_dictionary* dict,
shared_dictionary_type type,
std::size_t data_size,
const uint8_t data[]) const noexcept = 0;
-#endif
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct BOOST_SYMBOL_VISIBLE | |
| shared_dictionary_service | |
| { | |
| #if 0 | |
| virtual shared_dictionary* | |
| create_instance( | |
| alloc_func alloc_func, | |
| free_func free_func, | |
| void* opaque) const noexcept = 0; | |
| virtual void | |
| destroy_instance( | |
| shared_dictionary* dict) const noexcept = 0; | |
| virtual bool | |
| attach( | |
| shared_dictionary* dict, | |
| shared_dictionary_type type, | |
| std::size_t data_size, | |
| const uint8_t data[]) const noexcept = 0; | |
| #endif | |
| `#include` <boost/http/detail/config.hpp> | |
| `#include` <boost/http/brotli/types.hpp> | |
| `#include` <boost/capy/core/polystore_fwd.hpp> | |
| `#include` <cstdint> | |
| struct BOOST_SYMBOL_VISIBLE | |
| shared_dictionary_service | |
| { | |
| virtual shared_dictionary* | |
| create_instance( | |
| alloc_func alloc_func, | |
| free_func free_func, | |
| void* opaque) const noexcept = 0; | |
| virtual void | |
| destroy_instance( | |
| shared_dictionary* dict) const noexcept = 0; | |
| virtual bool | |
| attach( | |
| shared_dictionary* dict, | |
| shared_dictionary_type type, | |
| std::size_t data_size, | |
| const uint8_t data[]) const noexcept = 0; | |
| }; |
🤖 Prompt for AI Agents
In `@include/boost/http/brotli/shared_dictionary.hpp` around lines 43 - 63,
shared_dictionary_service currently has its virtual interface disabled by the
`#if 0` guard, which breaks the overrides in src_brotli/shared_dictionary.cpp
(create_instance, destroy_instance, attach) and makes
install_shared_dictionary_service unusable; remove the `#if 0`/`#endif` so the
virtual methods (create_instance, destroy_instance, attach) are declared, and
add an `#include <cstdint>` to this header so `uint8_t` is defined, or
alternatively keep the guard and remove/stop using the override methods in the
implementation to match the stubbed interface.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 190-204: The boost_http_zlib target lacks the same include
directories and compile feature settings as boost_http; update the
boost_http_zlib target (and likewise boost_http_brotli) to mirror
boost_http_setup_properties by adding target_compile_features(boost_http_zlib
PUBLIC cxx_constexpr) and target_include_directories(boost_http_zlib PUBLIC
"${PROJECT_SOURCE_DIR}/include" PRIVATE "${PROJECT_SOURCE_DIR}"), ensuring the
same PUBLIC/PRIVATE visibility and matching the settings applied to the
boost_http target.
- Around line 206-221: The brotli and zlib subtargets need explicit property
setup: after the add_library(...) for boost_http_brotli and boost_http_zlib,
call boost_http_setup_properties(<target>) before linking to boost_http so the
public include dirs, compile features and definitions are applied directly to
those targets; locate the add_library lines for boost_http_brotli and
boost_http_zlib and insert boost_http_setup_properties(boost_http_brotli) and
boost_http_setup_properties(boost_http_zlib) immediately after their respective
add_library(...) calls and before target_link_libraries(... PUBLIC boost_http).
♻️ Duplicate comments (2)
build/Jamfile (1)
68-95: Propagate external library dependencies to usage-requirements for static linking.The
boost_http_zlibandboost_http_brotlilibraries link against external libraries inrequirements, but these don't propagate to consumers. In static builds, dependents will encounter unresolved symbols because they won't receive the necessary linker flags.Add the external libraries to
usage-requirements:Proposed fix
lib boost_http_zlib : http_zlib_sources : requirements <library>/boost/http//boost_http <define>BOOST_HTTP_SOURCE [ ac.check-library /zlib//zlib : <library>/zlib//zlib : <build>no ] : usage-requirements <library>/boost/http//boost_http <define>BOOST_HTTP_HAS_ZLIB + <library>/zlib//zlib ; lib boost_http_brotli : http_brotli_sources : requirements <library>/boost/http//boost_http <define>BOOST_HTTP_SOURCE [ ac.check-library /brotli//brotlicommon : <library>/brotli//brotlicommon : <build>no ] [ ac.check-library /brotli//brotlidec : <library>/brotli//brotlidec : <build>no ] [ ac.check-library /brotli//brotlienc : <library>/brotli//brotlienc : <build>no ] : usage-requirements <library>/boost/http//boost_http <define>BOOST_HTTP_HAS_BROTLI + <library>/brotli//brotlicommon + <library>/brotli//brotlidec + <library>/brotli//brotlienc ;CMakeLists.txt (1)
200-201: Make compression library dependencies PUBLIC for correct static linking.
boost_http_zlibandboost_http_brotlilink their external dependencies asPRIVATE. In static builds, consumers won't receive the necessary linker flags, causing unresolved symbol errors.Proposed fix
- target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) + target_link_libraries(boost_http_zlib PUBLIC ZLIB::ZLIB)- target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) + target_link_libraries(boost_http_brotli PUBLIC Brotli::common Brotli::decoder Brotli::encoder)Also applies to: 217-218
| # Zlib | ||
| find_package(ZLIB) | ||
| if (ZLIB_FOUND) | ||
| file(GLOB_RECURSE BOOST_HTTP_ZLIB_HEADERS CONFIGURE_DEPENDS include/boost/http/zlib/*.hpp) | ||
| file(GLOB_RECURSE BOOST_HTTP_ZLIB_SOURCES CONFIGURE_DEPENDS src_zlib/*.cpp src_zlib/*.hpp) | ||
| source_group("" FILES "include/boost/http/zlib.hpp") | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/include/boost/http/zlib PREFIX "include" FILES ${BOOST_HTTP_ZLIB_HEADERS}) | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/src_zlib PREFIX "src" FILES ${BOOST_HTTP_ZLIB_SOURCES}) | ||
| add_library(boost_http_zlib include/boost/http/zlib.hpp build/Jamfile ${BOOST_HTTP_ZLIB_HEADERS} ${BOOST_HTTP_ZLIB_SOURCES}) | ||
| add_library(Boost::http_zlib ALIAS boost_http_zlib) | ||
| target_link_libraries(boost_http_zlib PUBLIC boost_http) | ||
| target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) | ||
| target_compile_definitions(boost_http_zlib PUBLIC BOOST_HTTP_HAS_ZLIB) | ||
| target_compile_definitions(boost_http_zlib PRIVATE BOOST_HTTP_SOURCE) | ||
| endif () |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "\.hpp$" include/ | grep -i zlib | head -20Repository: cppalliance/http
Length of output: 176
🏁 Script executed:
cd /tmp && find . -name "CMakeLists.txt" -type f 2>/dev/null | head -1 | xargs cat > /dev/null 2>&1; echo "ready"Repository: cppalliance/http
Length of output: 65
🏁 Script executed:
# Get the full CMakeLists.txt to understand the project structure
wc -l CMakeLists.txtRepository: cppalliance/http
Length of output: 78
🏁 Script executed:
# Check the boost_http target setup
sed -n '1,189p' CMakeLists.txt | tail -50Repository: cppalliance/http
Length of output: 2337
🏁 Script executed:
# Check the complete ZLIB section and surrounding context
sed -n '180,210p' CMakeLists.txtRepository: cppalliance/http
Length of output: 1546
🏁 Script executed:
sed -n '204,240p' CMakeLists.txtRepository: cppalliance/http
Length of output: 1575
Add target_include_directories and target_compile_features to boost_http_zlib target.
The boost_http_zlib target is missing the explicit configuration applied to boost_http. Add the following to ensure proper compilation:
target_compile_features(boost_http_zlib PUBLIC cxx_constexpr)
target_include_directories(boost_http_zlib PUBLIC "${PROJECT_SOURCE_DIR}/include")
target_include_directories(boost_http_zlib PRIVATE "${PROJECT_SOURCE_DIR}")This matches the setup in the boost_http_setup_properties() helper function (used for the main target) and should also be applied to boost_http_brotli for consistency.
🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 190 - 204, The boost_http_zlib target lacks the
same include directories and compile feature settings as boost_http; update the
boost_http_zlib target (and likewise boost_http_brotli) to mirror
boost_http_setup_properties by adding target_compile_features(boost_http_zlib
PUBLIC cxx_constexpr) and target_include_directories(boost_http_zlib PUBLIC
"${PROJECT_SOURCE_DIR}/include" PRIVATE "${PROJECT_SOURCE_DIR}"), ensuring the
same PUBLIC/PRIVATE visibility and matching the settings applied to the
boost_http target.
| # Brotli | ||
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") | ||
| find_package(Brotli) | ||
| if (Brotli_FOUND) | ||
| file(GLOB_RECURSE BOOST_HTTP_BROTLI_HEADERS CONFIGURE_DEPENDS include/boost/http/brotli/*.hpp) | ||
| file(GLOB_RECURSE BOOST_HTTP_BROTLI_SOURCES CONFIGURE_DEPENDS src_brotli/*.cpp src_brotli/*.hpp) | ||
| source_group("" FILES "include/boost/http/brotli.hpp") | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/include/boost/http/brotli PREFIX "include" FILES ${BOOST_HTTP_BROTLI_HEADERS}) | ||
| source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR}/src_brotli PREFIX "src" FILES ${BOOST_HTTP_BROTLI_SOURCES}) | ||
| add_library(boost_http_brotli include/boost/http/brotli.hpp build/Jamfile ${BOOST_HTTP_BROTLI_HEADERS} ${BOOST_HTTP_BROTLI_SOURCES}) | ||
| add_library(Boost::http_brotli ALIAS boost_http_brotli) | ||
| target_link_libraries(boost_http_brotli PUBLIC boost_http) | ||
| target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) | ||
| target_compile_definitions(boost_http_brotli PUBLIC BOOST_HTTP_HAS_BROTLI) | ||
| target_compile_definitions(boost_http_brotli PRIVATE BOOST_HTTP_SOURCE) | ||
| endif () |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if brotli headers are in the main include path or a separate directory
fd -t f "\.hpp$" -p "brotli" include/Repository: cppalliance/http
Length of output: 3419
🏁 Script executed:
# Check CMakeLists.txt for zlib configuration to see the pattern
rg -A 10 -B 2 "zlib" CMakeLists.txt | head -60Repository: cppalliance/http
Length of output: 1542
🏁 Script executed:
# Look for any target_include_directories calls in the entire CMakeLists.txt
rg "target_include_directories" CMakeLists.txt -A 2Repository: cppalliance/http
Length of output: 595
🏁 Script executed:
# Get more context around where target_include_directories is called
rg "target_include_directories" CMakeLists.txt -B 10 | head -80Repository: cppalliance/http
Length of output: 1010
🏁 Script executed:
# Check if there's a function or foreach loop that applies to all targets
rg "foreach|function|macro" CMakeLists.txt | head -20Repository: cppalliance/http
Length of output: 381
🏁 Script executed:
# Look at the complete zlib and brotli section to see full context
sed -n '184,230p' CMakeLists.txtRepository: cppalliance/http
Length of output: 2519
🏁 Script executed:
# Look for where boost_http_setup_properties is called
rg "boost_http_setup_properties" CMakeLists.txtRepository: cppalliance/http
Length of output: 195
🏁 Script executed:
# Get context around the main boost_http library creation
sed -n '160,190p' CMakeLists.txtRepository: cppalliance/http
Length of output: 1345
🏁 Script executed:
# Check the full CMakeLists.txt for main boost_http target setup
rg -B 5 "add_library\(boost_http" CMakeLists.txt | head -40Repository: cppalliance/http
Length of output: 1816
Add explicit property setup for boost_http_brotli (and boost_http_zlib) targets.
Both boost_http_brotli and boost_http_zlib should call boost_http_setup_properties() immediately after their add_library() call, just like the main boost_http target does. This ensures they have explicit public include directories, compile features, and definitions properly configured rather than relying solely on inheritance through target_link_libraries(... PUBLIC boost_http).
Example fix for brotli (apply same pattern to zlib)
add_library(boost_http_brotli include/boost/http/brotli.hpp build/Jamfile ${BOOST_HTTP_BROTLI_HEADERS} ${BOOST_HTTP_BROTLI_SOURCES})
add_library(Boost::http_brotli ALIAS boost_http_brotli)
boost_http_setup_properties(boost_http_brotli)
target_link_libraries(boost_http_brotli PUBLIC boost_http)
🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 206 - 221, The brotli and zlib subtargets need
explicit property setup: after the add_library(...) for boost_http_brotli and
boost_http_zlib, call boost_http_setup_properties(<target>) before linking to
boost_http so the public include dirs, compile features and definitions are
applied directly to those targets; locate the add_library lines for
boost_http_brotli and boost_http_zlib and insert
boost_http_setup_properties(boost_http_brotli) and
boost_http_setup_properties(boost_http_zlib) immediately after their respective
add_library(...) calls and before target_link_libraries(... PUBLIC boost_http).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 159-162: The hardcoded sibling include path using
PROJECT_SOURCE_DIR in the target_include_directories call is brittle; modify the
CMake logic to first check for that directory's existence (e.g., using if(EXISTS
${PROJECT_SOURCE_DIR}/../corosio/include)) and only add the BUILD_INTERFACE
include when present, otherwise fall back to a find_path lookup (e.g., find_path
to populate a COROSIO_INCLUDE_DIR) and add that result if found; keep the
target_compile_definitions(… BOOST_COROSIO_NO_LIB) behavior but conditionally
add the include only when a valid corosio include directory is discovered so
standalone http builds via find_package won't break.
♻️ Duplicate comments (3)
build/Jamfile (1)
70-97: External compression libraries still missing fromusage-requirements.Despite the prior review comment being marked as addressed, the current code still does not propagate the external library dependencies to consumers via
usage-requirements. This will cause unresolved symbol errors in static builds when dependents link againstboost_http_zliborboost_http_brotli.The
<library>/zlib//zliband the three brotli libraries need to be added to their respectiveusage-requirementssections.Proposed fix
lib boost_http_zlib : http_zlib_sources : requirements <library>/boost/http//boost_http <define>BOOST_HTTP_SOURCE [ ac.check-library /zlib//zlib : <library>/zlib//zlib : <build>no ] : usage-requirements <library>/boost/http//boost_http <define>BOOST_HTTP_HAS_ZLIB + <library>/zlib//zlib ; # Brotli using brotli ; alias http_brotli_sources : [ glob-tree-ex src_brotli : *.cpp ] ; lib boost_http_brotli : http_brotli_sources : requirements <library>/boost/http//boost_http <define>BOOST_HTTP_SOURCE [ ac.check-library /brotli//brotlicommon : <library>/brotli//brotlicommon : <build>no ] [ ac.check-library /brotli//brotlidec : <library>/brotli//brotlidec : <build>no ] [ ac.check-library /brotli//brotlienc : <library>/brotli//brotlienc : <build>no ] : usage-requirements <library>/boost/http//boost_http <define>BOOST_HTTP_HAS_BROTLI + <library>/brotli//brotlicommon + <library>/brotli//brotlidec + <library>/brotli//brotlienc ;CMakeLists.txt (2)
192-206: Missing property setup and incorrect link visibility for static builds.Two issues remain from prior review feedback:
Missing
boost_http_setup_properties(): The zlib target doesn't get explicit include directories or compile features. WhilePUBLIC boost_httppropagates some properties, callingboost_http_setup_properties(boost_http_zlib)ensures consistency.
ZLIB::ZLIBlinked as PRIVATE: In static builds, consumers linkingboost_http_zlibwon't receive ZLIB linker flags, causing unresolved symbol errors.Suggested fix
add_library(boost_http_zlib include/boost/http/zlib.hpp build/Jamfile ${BOOST_HTTP_ZLIB_HEADERS} ${BOOST_HTTP_ZLIB_SOURCES}) add_library(Boost::http_zlib ALIAS boost_http_zlib) + boost_http_setup_properties(boost_http_zlib) target_link_libraries(boost_http_zlib PUBLIC boost_http) - target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) + target_link_libraries(boost_http_zlib PUBLIC ZLIB::ZLIB) target_compile_definitions(boost_http_zlib PUBLIC BOOST_HTTP_HAS_ZLIB) target_compile_definitions(boost_http_zlib PRIVATE BOOST_HTTP_SOURCE)
208-223: Same issues as zlib target: missing property setup and PRIVATE linking.Apply the same fixes as the zlib target:
- Add
boost_http_setup_properties(boost_http_brotli)afteradd_library- Change Brotli dependencies to
PUBLICfor proper static linkingSuggested fix
add_library(boost_http_brotli include/boost/http/brotli.hpp build/Jamfile ${BOOST_HTTP_BROTLI_HEADERS} ${BOOST_HTTP_BROTLI_SOURCES}) add_library(Boost::http_brotli ALIAS boost_http_brotli) + boost_http_setup_properties(boost_http_brotli) target_link_libraries(boost_http_brotli PUBLIC boost_http) - target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) + target_link_libraries(boost_http_brotli PUBLIC Brotli::common Brotli::decoder Brotli::encoder) target_compile_definitions(boost_http_brotli PUBLIC BOOST_HTTP_HAS_BROTLI) target_compile_definitions(boost_http_brotli PRIVATE BOOST_HTTP_SOURCE)
🧹 Nitpick comments (2)
build/Jamfile (1)
68-68: Mark source aliases asexplicitfor consistency.The
http_sourcesalias on line 38 is markedexplicitto prevent it from being built by default. The new source aliaseshttp_zlib_sourcesandhttp_brotli_sourcesshould follow the same pattern for consistency.Proposed fix
alias http_zlib_sources : [ glob-tree-ex src_zlib : *.cpp ] ; + +explicit http_zlib_sources ;alias http_brotli_sources : [ glob-tree-ex src_brotli : *.cpp ] ; + +explicit http_brotli_sources ;Also applies to: 84-84
CMakeLists.txt (1)
200-206: MissingBOOST_HTTP_DYN_LINK/BOOST_HTTP_STATIC_LINKdefinitions.Both
boost_http_zlibandboost_http_brotlisetBOOST_HTTP_SOURCEprivately but don't defineBOOST_HTTP_DYN_LINKorBOOST_HTTP_STATIC_LINKbased onBUILD_SHARED_LIBS. This could cause symbol visibility issues.Calling
boost_http_setup_properties()(as suggested above) would resolve this since it handles these definitions automatically.Also applies to: 217-223
| # Add corosio headers without linking (header-only usage) | ||
| # BUILD_INTERFACE: only during build (installed headers are in standard Boost include path) | ||
| target_include_directories(${target} PUBLIC $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/../corosio/include>) | ||
| target_compile_definitions(${target} PUBLIC BOOST_COROSIO_NO_LIB) # Disable corosio auto-linking |
There was a problem hiding this comment.
Hardcoded sibling path may break standalone builds.
The path ${PROJECT_SOURCE_DIR}/../corosio/include assumes corosio is always a sibling directory. This works in Boost super-project builds but may fail for users building http standalone (e.g., via find_package).
Consider guarding this with a check for the directory's existence or using find_path as a fallback:
Suggested improvement
- # Add corosio headers without linking (header-only usage)
- # BUILD_INTERFACE: only during build (installed headers are in standard Boost include path)
- target_include_directories(${target} PUBLIC $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/../corosio/include>)
+ # Add corosio headers without linking (header-only usage)
+ # BUILD_INTERFACE: only during build (installed headers are in standard Boost include path)
+ set(COROSIO_INCLUDE_DIR "${PROJECT_SOURCE_DIR}/../corosio/include")
+ if (EXISTS "${COROSIO_INCLUDE_DIR}")
+ target_include_directories(${target} PUBLIC $<BUILD_INTERFACE:${COROSIO_INCLUDE_DIR}>)
+ endif ()🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 159 - 162, The hardcoded sibling include path
using PROJECT_SOURCE_DIR in the target_include_directories call is brittle;
modify the CMake logic to first check for that directory's existence (e.g.,
using if(EXISTS ${PROJECT_SOURCE_DIR}/../corosio/include)) and only add the
BUILD_INTERFACE include when present, otherwise fall back to a find_path lookup
(e.g., find_path to populate a COROSIO_INCLUDE_DIR) and add that result if
found; keep the target_compile_definitions(… BOOST_COROSIO_NO_LIB) behavior but
conditionally add the include only when a valid corosio include directory is
discovered so standalone http builds via find_package won't break.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 421-427: The workflow currently uses a constant-false guard (if:
false) on the "Find Package Integration Workflow" step (uses:
alandefreitas/cpp-actions/cmake-workflow@v1.9.0), which triggers actionlint;
replace the constant with a runtime-controlled boolean (e.g., a
workflow_dispatch input or repo/workflow env var) and update the if to reference
that variable (for example if: ${{ inputs.run_find_package == 'true' }} or if:
${{ env.RUN_FIND_PACKAGE == 'true' }}), and add the corresponding input/env
default (false) so the step is disabled by default but no longer a constant
expression.
♻️ Duplicate comments (2)
CMakeLists.txt (2)
162-165: Guard the sibling corosio include path for standalone builds.
"${PROJECT_SOURCE_DIR}/../corosio/include"assumes a sibling layout and can break non-superproject builds. Consider checking existence (or afind_pathfallback) before adding it.🔧 Suggested guard
- target_include_directories(${target} PUBLIC $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/../corosio/include>) + set(COROSIO_INCLUDE_DIR "${PROJECT_SOURCE_DIR}/../corosio/include") + if (EXISTS "${COROSIO_INCLUDE_DIR}") + target_include_directories(${target} PUBLIC $<BUILD_INTERFACE:${COROSIO_INCLUDE_DIR}>) + endif ()
195-209: Propagate ZLIB/Brotli link deps to consumers (static builds).
WithPRIVATElinkage, consumers ofBoost::http_zlib/Boost::http_brotlimay miss required linker flags in static builds. Make these dependenciesPUBLIC.🔧 Suggested fix
- target_link_libraries(boost_http_zlib PUBLIC boost_http) - target_link_libraries(boost_http_zlib PRIVATE ZLIB::ZLIB) + target_link_libraries(boost_http_zlib PUBLIC boost_http ZLIB::ZLIB) @@ - target_link_libraries(boost_http_brotli PUBLIC boost_http) - target_link_libraries(boost_http_brotli PRIVATE Brotli::common Brotli::decoder Brotli::encoder) + target_link_libraries(boost_http_brotli PUBLIC boost_http Brotli::common Brotli::decoder Brotli::encoder)Also applies to: 211-226
Add zlib and brotli compression services from capy. These services provide HTTP content encoding support (gzip, deflate, br) and are better placed in the http library where they are actually used. - Add all zlib/brotli headers, sources, and tests - Change namespace from boost::capy to boost::http - Add compression documentation with cross-references - Update build files (CMake, B2, CI) - Update parser/serializer to use new namespace
MungoG
force-pushed
the
zlib-brotli-migration
branch
from
daafbc4 to
1e3795b
Compare
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.