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

[Noah Shutty via Phabricator via llvm-commits]

noajshu added a comment.

If we do want to merge the CulrHTTPClient into the base HTTPClient so that the default class just uses curl, what's the preferable way to do this? One option is just to move all code in `CurlHTTPClient.cpp` into `HTTPClient.cpp`, wrapping it in a `#ifdef LLVM_ENABLE_CURL`.



================
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);
----------------
labath wrote:
> 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.
Good catch. That was a mistake. There are no end-to-end tests running actual curl requests so I did not catch this. The only test checks the debuginfod client.

I like the idea of storing the error in the CurlHTTPRequest. I have implemented this change.


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

https://reviews.llvm.org/D112753