Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
  • Loading branch information
LinZhihao-723 and kirkrodrigues authored Jul 25, 2024
1 parent f035a57 commit d9961bd
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 10 deletions.
8 changes: 4 additions & 4 deletions components/core/src/clp/CurlGlobalInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace clp {
CurlGlobalInstance::CurlGlobalInstance() {
std::lock_guard<std::mutex> const global_lock{m_global_mutex};
if (0 == m_num_living_instances) {
if (auto const err{curl_global_init(CURL_GLOBAL_ALL)}; CURLE_OK != err) {
if (auto const err{curl_global_init(CURL_GLOBAL_ALL)}; 0 != err) {
throw CurlOperationFailed(
ErrorCode_Failure,
__FILE__,
Expand All @@ -30,9 +30,9 @@ CurlGlobalInstance::~CurlGlobalInstance() {
if (0 == m_num_living_instances) {
#if defined(__APPLE__)
// NOTE: On macOS, calling `curl_global_init` after `curl_global_cleanup` will fail with
// CURLE_SSL_CONNECT_ERROR. Thus, for now, we skip `deinit` on macOS. Related issues:
// - https://github.com/curl/curl/issues/12525
// - https://github.com/curl/curl/issues/13805
// CURLE_SSL_CONNECT_ERROR. Thus, for now, we skip `deinit` on macOS. Luckily, it is safe to
// call `curl_global_init` multiple times without calling `curl_global_cleanup`. Related
// issues:
// TODO: Remove this conditional logic when the issues are resolved.
return;
#else
Expand Down
9 changes: 5 additions & 4 deletions components/core/src/clp/CurlGlobalInstance.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@
namespace clp {
/**
* Class to wrap `libcurl`'s global initialization/de-initialization calls using RAII. Before using
* any `libcurl` functionalities, an instance of this function must be created to ensure underlying
* `libcurl` resources have been initialized. This class maintains a static reference count to all
* the living instances. De-initialization happens when the reference count reaches 0.
* any `libcurl` functionalities, an instance of this class must be created. Although unnecessasry,
* it can be safely instantiated multiple times; it maintains a static reference count to all
* existing instances and only di-initializes `libcurl`'s global resources when the reference count
* reaches 0.
*/
class CurlGlobalInstance {
public:
// Constructors
CurlGlobalInstance();

// Disable copy/move constructors/assignment operators
// Disable copy/move constructors and assignment operators
CurlGlobalInstance(CurlGlobalInstance const&) = delete;
CurlGlobalInstance(CurlGlobalInstance&&) = delete;
auto operator=(CurlGlobalInstance const&) -> CurlGlobalInstance& = delete;
Expand Down
4 changes: 2 additions & 2 deletions components/core/src/clp/NetworkReader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ class NetworkReader : public ReaderInterface {

/**
* Constructs a reader to stream data from the given URL, starting at the given offset.
* Note: This class depends on `libcurl`. An instance of `clp::CurlGlobalInstance` must remain
* alive for the entire lifespan of any instance of this class to maintain proper functionality.
* NOTE: This class depends on `libcurl`, so an instance of `clp::CurlGlobalInstance` must
* remain alive for the entire lifespan of any instance of this class.
* @param src_url
* @param offset Index of the byte at which to start the download
* @param disable_caching Whether to disable the caching.
Expand Down

0 comments on commit d9961bd

Please sign in to comment.