Skip to content

Conversation

@AenBleidd
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings October 26, 2025 03:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the HTTP CURL implementation by converting global state into a singleton class and replaces legacy C-style callbacks with C++ lambdas. It also adds comprehensive unit tests for the new HTTP_CURL class.

  • Converted global variables and functions into the HTTP_CURL singleton class
  • Replaced C-style callback functions with lambda expressions
  • Added unit tests covering singleton behavior, thread safety, and HTTP protocol flags

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/unit-tests/client/test_http_curl.cpp New unit test file with tests for HTTP_CURL singleton methods and thread-safe trace ID generation
tests/unit-tests/client/CMakeLists.txt New CMake configuration for client unit tests, includes all necessary client source files and dependencies
tests/unit-tests/CMakeLists.txt Updated to add client subdirectory and required dependencies (CURL, ZLIB, OpenSSL, X11)
tests/executeUnitTests.sh Updated test execution script to include client tests for both Darwin and other platforms
client/main.cpp Removed explicit curl_init() and curl_cleanup() calls as they're now managed by HTTP_CURL singleton
client/http_curl.h Added HTTP_CURL singleton class definition with thread-safe atomic counters, removed global function declarations
client/http_curl.cpp Refactored to use HTTP_CURL singleton, converted callback functions to lambdas, improved type safety with static_cast

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
HTTP_CURL::HTTP_CURL() :
curlMulti(curl_init()),
user_agent_string(std::move(get_user_agent_string())) {
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling the member function get_user_agent_string() during initialization is unsafe because it accesses the user_agent_string member variable before initialization completes. Move the initialization logic to the constructor body or ensure get_user_agent_string() doesn't reference uninitialized members.

Suggested change
user_agent_string(std::move(get_user_agent_string())) {
user_agent_string() {
user_agent_string = get_user_agent_string();

Copilot uses AI. Check for mistakes.
curl_easy_setopt(curlEasy, CURLOPT_DEBUGDATA, this );
curl_easy_setopt(curlEasy, CURLOPT_DEBUGFUNCTION,
[](CURL*, curl_infotype type, char *data, size_t, HTTP_OP* phop) {
if (!log_flags.http_debug) return;
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lambda has return type deduced from first return statement (line 693), which returns void, but line 707 attempts to return without a value which is inconsistent. The return type should be explicitly specified as 'int' and line 707 should return 0 to match the original function signature expected by CURLOPT_DEBUGFUNCTION.

Copilot uses AI. Check for mistakes.
"[http] [ID#%u] %s %s\n", phop->trace_id, desc, line.c_str()
);
}
return;
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return statement is unreachable because all switch cases either break or return, and the code after the switch is also a return. Remove this redundant return statement.

Copilot uses AI. Check for mistakes.

set (CMAKE_CXX_STANDARD 17)
set (CMAKE_CXX_FLAGS "-g -Wall -Wextra -Werror --coverage")
#set (CMAKE_CXX_FLAGS "-g -Wall -Wextra -Werror --coverage")
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented-out line with -Werror should be removed rather than kept as a comment. If -Werror needs to be disabled temporarily, document the reason in a comment.

Suggested change
#set (CMAKE_CXX_FLAGS "-g -Wall -Wextra -Werror --coverage")
# -Werror is currently omitted to allow compilation with warnings during development and CI.

Copilot uses AI. Check for mistakes.
Comment on lines 218 to 219
HTTP_CURL(HTTP_CURL const&); // must be private and unimplemented
void operator=(HTTP_CURL const&); // must be private and unimplemented
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In C++11 and later, use '= delete' syntax instead of declaring copy constructor and assignment operator as private and unimplemented. Change to 'HTTP_CURL(HTTP_CURL const&) = delete;' and 'void operator=(HTTP_CURL const&) = delete;'

Suggested change
HTTP_CURL(HTTP_CURL const&); // must be private and unimplemented
void operator=(HTTP_CURL const&); // must be private and unimplemented
HTTP_CURL(HTTP_CURL const&) = delete;
void operator=(HTTP_CURL const&) = delete;

Copilot uses AI. Check for mistakes.
Signed-off-by: Vitalii Koshura <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant