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

[Noah Shutty via Phabricator via llvm-commits]

noajshu added a comment.

@labath Thank you for your helpful feedback on this diff!
I have split into 4 sub-diffs. The HTTP client/request/response handler architecture is in D112751 <https://reviews.llvm.org/D112751> and the curl client is in D112753 <https://reviews.llvm.org/D112753>. Could we continue the discussion there?



================
Comment at: llvm/include/llvm/Support/HTTPClient.h:79-81
+  virtual Error curlInit();
+  virtual Error curlPerform();
+  virtual void curlCleanup();
----------------
phosek wrote:
> Do these need to public?
Nope -- updated most to private, and `curlInit` to protected (for unit testing).


================
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);
----------------
phosek wrote:
> Do these need to be public?
The `curlHandle{HeaderLine,BodyChunk}` method is invoked by the static `curl{HeaderLine,BodyChunk}MethodHook` function, which then needs access.

I tried instead having curl directly call back to a private method, but then the implicit `this` argument is replaced by the first parameter passed by curl, which is the pointer to the header/body contents.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:95
+
+Expected<HTTPResponseBuffer> httpGet(const Twine &Url);
+
----------------
phosek wrote:
> 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).
Agreed that this could be removed and on the flexibility point I bet another use case might be a `StreamingHTTPResponseHandler`. This would be especially nice if the `localCache` is refactored to allow caching objects chunk-by-chunk -- as then large debuginfo would not hog memory (as with the `BufferedHTTPResponseHandler`).


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

https://reviews.llvm.org/D111252