[PATCH] D112751: [llvm] [Support] Add HTTP Client Support library.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D112751#3120753 <https://reviews.llvm.org/D112751#3120753>, @labath wrote:

> In D112751#3119625 <https://reviews.llvm.org/D112751#3119625>, @dblaikie wrote:
>
>> @labath - what's your state on this and the subsequent patches? What would be best for me to focus on/do here to help out?
>
> I've pretty much done all I can with this series. I am happy with the code quality. Test coverage is ok. There's one ongoing discussion in https://reviews.llvm.org/D112753#inline-1083341, that I'd like to hear your thoughts on (if you have any), but that's a fairly small detail.
>
> I'd say the main thing that's missing is for someone to say "this belongs in llvm".

Fair enough - Yeah, I think the direction's worth having a go at - be nice to have it supported in llvm-symbolizer, etc.



================
Comment at: llvm/include/llvm/Support/HTTPClient.h:31
+  bool FollowRedirects = true;
+  bool operator==(const HTTPRequest &O) const {
+    return Url == O.Url && Method == O.Method &&
----------------
Usually best to write op overloads as non-members (possibly as friends, if that's necessary/helpful) to avoid different conversion candidates an the LHS and RHS.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:35
+  }
+  HTTPRequest(const StringRef Url);
+};
----------------
Skip const here


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:107
+  /// request to the given Url. Returns an HTTPResponseBuffer or an Error.
+  Expected<HTTPResponseBuffer> get(const StringRef Url);
+};
----------------
Skip top-level const here - while it's generally out of character for LLVM code, it's also not part of the function declaration (const here doesn't change the type of the function/doesn't change how people call it, it's only an implementation detail)


================
Comment at: llvm/lib/Support/HTTPClient.cpp:25
+
+HTTPRequest::HTTPRequest(const StringRef Url) { this->Url = Url.str(); }
+
----------------
Skip top-level "const" here - it's out of character for LLVM code.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:74
+
+Expected<HTTPResponseBuffer> HTTPClient::get(const StringRef Url) {
+  HTTPRequest Request(Url);
----------------
skip const here


================
Comment at: llvm/lib/Support/HTTPClient.cpp:79-81
+HTTPClient::HTTPClient() {}
+
+HTTPClient::~HTTPClient() {}
----------------
Use `= default;` here, perhaps?


================
Comment at: llvm/lib/Support/HTTPClient.cpp:85-89
+bool HTTPClient::IsInitialized = false;
+
+void HTTPClient::initialize() { IsInitialized = true; }
+
+void HTTPClient::cleanup() { IsInitialized = false; }
----------------
Is there any use for this variable? It's currently only written and never read, by the looks of it. Maybe that's worth deferring until some use is implemented.


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

https://reviews.llvm.org/D112751