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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D112753#3104614 <https://reviews.llvm.org/D112753#3104614>, @noajshu wrote:

> 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`.

I'm not aware of an exact precedent (due to this introducing a class instead of a bunch of free functions, etc.), but if I try to apply what happens with other external dependencies (see e.g. `Compression.h`) I get a `static bool (CURL)HTTPClient::isAvailable()` function, and an `llvm_unreachable` inside all of the member functions (when curl is unavailable).



================
Comment at: llvm/include/llvm/Support/CurlHTTPClient.h:20
+
+typedef void CurlHandle;
+
----------------
This definitely shouldn't be at the top level, but I'm not sure if you need it at all (I count exactly one usage of that).


================
Comment at: llvm/include/llvm/Support/CurlHTTPClient.h:35
+  /// Performs the request
+  Error perform(const HTTPRequest Request,
+                HTTPResponseHandler &Handler) override;
----------------
did you want a reference here? Plain const on a by-value argument is fairly useless.


================
Comment at: llvm/lib/Support/CurlHTTPClient.cpp:32
+  CurlHTTPRequest(HTTPResponseHandler &Handler) : Handler(Handler) {}
+  void storeError(Error &Err) {
+    ErrorState = joinErrors(std::move(Err), std::move(ErrorState));
----------------
remove this, and have caller use std::move


================
Comment at: llvm/lib/Support/CurlHTTPClient.cpp:97
+                                        curl_easy_strerror(CurlRes)));
+  else if (Error Err = std::move(CurlRequest.ErrorState))
+    return Err;
----------------
`/*no else*/ if (CurlRequest.ErrorState) return CurlRequest.ErrorState;` ?


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

https://reviews.llvm.org/D112753