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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I've found the error handling confusing. If the intention is that the `HTTPResponseHandler` can terminate the request by returning zero, then it seems to me that it would be better to have it return llvm::Error, and have the HTTPClient class translate that to whatever action is required ( == storing the error in some variable and returning 0 to curl). That would make the BufferedHTTPResponse class cleaner, as it would no longer have to deal with an Error member, and the overall api would be more understandable.



================
Comment at: llvm/include/llvm/Support/HTTPClient.h:40
+/// A handler for state updates during the lifecycle of a request performed
+/// by HTTPClient::perform. An HTTP ResponseHandler cannot fail,
+class HTTPResponseHandler {
----------------
Should this be a period, or is something missing?


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:44
+  /// Processes one line of HTTP response headers and returns the number of
+  /// bytes handled. Returning 0 causes the request to abort.
+  virtual size_t handleHeaderLine(StringRef HeaderLine) = 0;
----------------
I'm having trouble reconciling this with the "cannot fail" comment above. Can you elaborate on what the two comments mean?


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:70-75
+  /// Tracks any errors which have occurred during request handling, such as
+  /// if handleBodyChunk is called before a Content-Length header.
+  Error Err = Error::success();
+
+  /// Stores the data received from the HTTP server.
+  HTTPResponseBuffer ResponseBuffer;
----------------
Having an Error member is strange enough (because of it's destruction semantics), but making it a public member is unheard of. It would be much better to make these private and have a member function returning `Expected<HTTPResponseBuffer>`.

But it would be much better to not have the Error member in the first place.


================
Comment at: llvm/include/llvm/Support/HTTPClient.h:94
+  /// Returns any errors which occur during request.
+  /// If any of the callbacks to Handler return an Error, a Client
+  /// should stop performing the request and return an Error.
----------------
I don't see any methods returning an error


================
Comment at: llvm/lib/Support/HTTPClient.cpp:50
+  return LineRef.consume_front("Content-Length: ") &&
+         !getAsUnsignedInteger(LineRef.trim(), 10, ContentLength);
+}
----------------
if you used `llvm::to_integer` here, you would be able to use any type you want (size_t perhaps), and would get more natural failure semantics (no need for `!` -- `to_integer` returns true on success).


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:42-45
+  EXPECT_EQ(memcmp(Handler.ResponseBuffer.Body->getBufferStart(),
+                   "body:this puts the total at 36 chars",
+                   Handler.ResponseBuffer.Body->getBufferSize()),
+            0);
----------------
EXPECT_EQ(Handler.ResponseBuffer.Body->getBuffer(), "...");


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:157-162
+  MockHTTPResponseHandler Handler;
+  MockHTTPClient Client;
+  HTTPRequest Request("");
+  EXPECT_THAT_ERROR(Client.perform(Request, Handler), Succeeded());
+  EXPECT_EQ(Handler.HeaderLinesTotal, 1u);
+  EXPECT_EQ(Handler.BodyBytesTotal, 7u);
----------------
What is this supposed to test? This test pretty much only executes code which is defined in the test itself...


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

https://reviews.llvm.org/D112751