[PATCH] D112753: [llvm] [Support] Add CURL HTTP Client.

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/lib/Support/CurlHTTPClient.cpp:45-46
+  Size *= NMemb;
+  size_t Offset = CurlRequest->Offset;
+  CurlRequest->Offset += Size;
+  return CurlRequest->Handler->handleBodyChunk(StringRef(Contents, Size),
----------------
I'm not sure if we need this bookkeeping here. It makes the API redundant, and the same logic could be implemented in BufferedHTTPResponseHandler.


================
Comment at: llvm/lib/Support/CurlHTTPClient.cpp:51-54
+template <typename T1, typename T2>
+void CurlHTTPClient::curlSetOpt(T1 Option, T2 Parameter) {
+  curl_easy_setopt(Curl, Option, Parameter);
+}
----------------
I don't think this function adds any value now...


================
Comment at: llvm/lib/Support/CurlHTTPClient.cpp:81-82
+  curlSetOpt(CURLOPT_FOLLOWLOCATION, Request.FollowRedirects);
+  curlSetOpt(CURLOPT_WRITEDATA, &Handler);
+  curlSetOpt(CURLOPT_HEADERDATA, &Handler);
+  CURLcode CurlRes = curl_easy_perform(Curl);
----------------
Are you sure this works? I was expecting to see a `CurlHTTPRequest` object here.

And the reason I am looking at this is because I was wondering if we could store the llvm::Error (from the previous commit) inside `CurlHTTPRequest`. That way an Error member wouldn't be a part of any public API, and here its lifetime would be clearly limited to the duration of this function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112753/new/

https://reviews.llvm.org/D112753