Using pImpl to avoid the usage of void* #2

afrizaloky · 2024-07-31T16:25:28Z

Using void* to hide the real implementation is not recommended because it will stop the compiler from being able to enforce type checking
Add unittest using gtest

Reference

https://en.cppreference.com/w/cpp/language/pimpl

ammarfaizi2 · 2024-08-01T08:05:25Z

Please split the first commit into smaller manageable commits; each should focus on one thing.

This PR should at least contain 4 commits:

Wrap the CURL pointer.
Add CMake support.
Add unit test.
Add GitHub action.

Don't do too many things at once.

In addition to that:
When applied, each commit should not leave a known broken function. So, the order matters if a commit depends on a previous commit. For example, no (1) can be reordered freely, but (2) and (3) can't, as (2) must come first because the unit test needs CMake.

ammarfaizi2 · 2024-08-01T08:20:23Z

src/LokalSo/Lokal.cpp

@@ -143,18 +142,15 @@ std::string Lokal::__curl(std::string path, std::string method,
 	curl_easy_setopt(ch, CURLOPT_HEADERDATA, &data);
 	curl_easy_setopt(ch, CURLOPT_USERAGENT, "Lokal Go - github.com/lokal-so/lokal-go");
 	curl_easy_setopt(ch, CURLOPT_VERBOSE, 0L);
-	curl_easy_setopt(ch, CURLOPT_HTTPHEADER, headers);
+	curl_easy_setopt(ch, CURLOPT_HTTPHEADER, headers.get());


This header wrapping thing is gruesome. Plus, I don't see the need to
do that. The slist_append and slist_free here are never touched by
the exception.

It costs extra complexity and an indirect call to the deleter without
code improvement.

If you really want to wrap it up, create the whole CURL class that
handles the header addition. Like:

class Lokal { private: CURLImpl c_; }; std::string Lokal::curl(/* params */) { c_->setURL(url); c_->addHeader("Content-Type: application/json"); c_->setOpt(/* ... */) }

... and then when the Lokal object is destroyed, the CURLImpl will
also be destroyed cleanly withouth the need for slist_free and
slist_append from the caller (do those internally in CURLImpl).

Something like that is the expected improvement, not just blindly
wrapping everything with unique_ptr. Otherwise, keep it as is in
the C fashioned way.

ammarfaizi2 · 2024-08-01T08:56:10Z

@afrizaloky if you want to wrap the CURL pointer. Please do the following requirements.

[ Just for fun, I asked ChatGPT and it yields a nice result. But I am not going to use ChatGPT's code. ]

Create a curl class named CurlImpl in C++ that wraps the libcurl
function from curl/curl.h header.

Key features:

f: setURL
f: addHeader
f: setMethod
f: setReqBody
f: setUserAgent
f: execute (throws an exception when error)
f: getResHeader (returns vec<string, string>)
f: getResBody (returns string)

Additional notes:

The CurlImpl object must be reusable for multiple HTTP requests.
Keep the connection alive as long as possible (only destroy CURL *
pointer in the destructor).
The default method is GET.
Each execute() call must reset method, req body, and req headers
(except for CURLOPT_USERAGENT, keep it as is).

[ Edit: Fix (7) and (8) names. ]

1. Using void* to hide the real implementation is not recommended because it will stop the compiler from being able to enforce type checking 2. Create CurlWrapper to wrap the implementation of cURL 3. Create CurlImpl to hide the implementation of CurlWrapper, so the main header doesnt need to include the header

ammarfaizi2

Please follow the coding style around the code.

Indentation: Tab
Tab width: 8

ammarfaizi2 · 2024-08-03T08:29:59Z

src/LokalSo/CurlWrapper.cpp

+namespace LokalSo {
+
+    static void ltrim(std::string &s) {
+        s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](unsigned char ch) {
+            return !std::isspace(ch);
+        }));
+    }
+
+    static void rtrim(std::string &s) {
+        s.erase(std::find_if(s.rbegin(), s.rend(), [](unsigned char ch) {
+            return !std::isspace(ch);


Don't add indentation inside the namespace.

ammarfaizi2 · 2024-08-03T08:31:43Z

src/LokalSo/CurlWrapper.cpp

+
+    CurlWrapper::CurlWrapper() {
+        this->ch = curl_easy_init();
+        curl_easy_setopt(this->ch, CURLOPT_WRITEFUNCTION, &res_body_callback);
+	    curl_easy_setopt(this->ch, CURLOPT_WRITEDATA, this);
+
+        curl_easy_setopt(this->ch, CURLOPT_HEADERFUNCTION, &res_hdr_callback);
+	    curl_easy_setopt(this->ch, CURLOPT_HEADERDATA, this);
+
+    }
+    CurlWrapper::~CurlWrapper() {
+        if(this->ch) {
+            curl_easy_cleanup(this->ch);
+        }
+        if(this->sList) {
+            curl_slist_free_all(this->sList);
+        }
+    }


Fix the bad indentation and add an empty line between function definitions.

ammarfaizi2 · 2024-08-03T08:32:30Z

src/LokalSo/CurlWrapper.cpp

+
+
+        curl_easy_reset(this->ch);
+	    // curl_easy_setopt(this->ch, CURLOPT_URL, nullptr);
+        // curl_easy_setopt(this->ch, CURLOPT_POST, 0L);
+		// curl_easy_setopt(this->ch, CURLOPT_POSTFIELDSIZE, 0);
+		// curl_easy_setopt(this->ch, CURLOPT_POSTFIELDS, nullptr);
+    	// curl_easy_setopt(ch, CURLOPT_HTTPHEADER, nullptr);
+    }


Kill these unnecessary comments.

ammarfaizi2 · 2024-08-03T08:35:02Z

src/LokalSo/CurlWrapper.cpp

+    static size_t res_hdr_callback(void *contents, size_t size, size_t nmemb,
+			       void *userp) {
+        CurlWrapper* mem = static_cast<CurlWrapper*>(userp);
+        size_t full_size = size * nmemb;
+
+        std::string headerStr(static_cast<char *>(contents), full_size);
+
+        auto colon = headerStr.find(':');
+        if (colon != std::string::npos) {
+            std::string key = headerStr.substr(0, colon);
+            std::string value = headerStr.substr(colon + 1);
+            // Trim whitespace from key and value
+            trim(key);
+            trim(value);
+            mem->addResponseHeader(key, value);


Apart from the trim, make the key lowercase (only for the response header).

ammarfaizi2 reviewed Aug 1, 2024

View reviewed changes

afrizaloky force-pushed the dev/pimpl branch from a42cb04 to d5dbe96 Compare August 2, 2024 09:48

afrizaloky force-pushed the dev/pimpl branch from d5dbe96 to fad98c4 Compare August 2, 2024 09:49

Fix missing error handling if status code is not 200

3622500

ammarfaizi2 requested changes Aug 3, 2024

View reviewed changes

afrizaloky added 4 commits August 3, 2024 21:45

Fix indentation

e40af4d

Add to lower

f2867e8

Fix missing add header to curl

6742d5e

Fix: wrong calling method

199d0fa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using pImpl to avoid the usage of void* #2

Using pImpl to avoid the usage of void* #2

afrizaloky commented Jul 31, 2024

ammarfaizi2 commented Aug 1, 2024 •

edited

Loading

ammarfaizi2 Aug 1, 2024 •

edited

Loading

ammarfaizi2 commented Aug 1, 2024 •

edited

Loading

ammarfaizi2 left a comment

ammarfaizi2 Aug 3, 2024

ammarfaizi2 Aug 3, 2024

ammarfaizi2 Aug 3, 2024

ammarfaizi2 Aug 3, 2024

Using pImpl to avoid the usage of void* #2

Are you sure you want to change the base?

Using pImpl to avoid the usage of void* #2

Conversation

afrizaloky commented Jul 31, 2024

ammarfaizi2 commented Aug 1, 2024 • edited Loading

ammarfaizi2 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

ammarfaizi2 commented Aug 1, 2024 • edited Loading

ammarfaizi2 left a comment

Choose a reason for hiding this comment

ammarfaizi2 Aug 3, 2024

Choose a reason for hiding this comment

ammarfaizi2 Aug 3, 2024

Choose a reason for hiding this comment

ammarfaizi2 Aug 3, 2024

Choose a reason for hiding this comment

ammarfaizi2 Aug 3, 2024

Choose a reason for hiding this comment

ammarfaizi2 commented Aug 1, 2024 •

edited

Loading

ammarfaizi2 Aug 1, 2024 •

edited

Loading

ammarfaizi2 commented Aug 1, 2024 •

edited

Loading