[PATCH] D111252: [llvm] [Support] [Debuginfo] Add http and debuginfod client libraries and llvm-debuginfod-find tool

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:41
+class BufferedHTTPResponseHandler : public HTTPResponseHandler {
+private:
+  size_t BufferOffset = 0;
----------------
This is unnecessary.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:70-93
+class CurlHTTPRequest : public HTTPRequest {
+private:
+  CURL *Curl = nullptr;
+
+  template <typename T> void curlSetOpt(CURLoption Option, T Parameter) {
+    curl_easy_setopt(this->Curl, Option, Parameter);
+  }
----------------
I'd move the cURL-based implementation into a separate header and source file, for example `Support/CURLClient.{h,cpp}`. Otherwise, anyone who includes `HTTPClient.h` automatically pulls in `<curl/curl.h>` which is undesirable.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:71
+class CurlHTTPRequest : public HTTPRequest {
+private:
+  CURL *Curl = nullptr;
----------------
This is unnecessary.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:79-81
+  virtual Error curlInit();
+  virtual Error curlPerform();
+  virtual void curlCleanup();
----------------
Do these need to public?


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:83-86
+  size_t curlHandleHeaderLine(char *Contents, size_t Size, size_t NMemb,
+                              void * /*Unused*/ Ptr);
+  size_t curlHandleBodyChunk(char *Contents, size_t Size, size_t NMemb,
+                             void * /*Unused*/ Ptr);
----------------
Do these need to be public?


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:95
+
+Expected<HTTPResponseBuffer> httpGet(const Twine &Url);
+
----------------
This function seems unnecessary as it's functionality is now effectively provided by `HTTPRequest` and `HTTPRequestConfig` which is more flexible (for example they can potentially support methods other than Get).


================
Comment at: llvm/lib/Support/HTTPClient.cpp:77
+  if (Error Err = SizeOrErr.takeError()) {
+    errs() << Err << "\n";
+    return 0;
----------------
Rather than printing the error to stderr which may be undesirable for example when using LLVM as a library, I think it'd be better to store the error inside the class (it might be possible to use `ErrorList`) and then return it from `curlPerform`.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:89
+  if (Error Err = SizeOrErr.takeError()) {
+    errs() << Err << "\n";
+    return 0;
----------------
The same here.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:103-113
+Error llvm::CurlHTTPRequest::curlPerform() {
+  CURLcode CurlRes = curl_easy_perform(Curl);
+  if (CurlRes != CURLE_OK)
+    return createStringError(errc::io_error, "curl_easy_perform() failed: %s\n",
+                             curl_easy_strerror(CurlRes));
+
+  unsigned Code;
----------------
Could this method be inlined directly into `CurlHTTPRequest::performRequest`?


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

https://reviews.llvm.org/D111252