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

[Noah Shutty via Phabricator via llvm-commits]

noajshu added a comment.

@labath I completely agree the new error handling is awkward and I prefer your proposed Error handling between the ResponseHandler and the Client. I'll change all three handle*() methods to return Error (then convert it to 0 for curl in the client + hook functions)



================
Comment at: llvm/lib/Support/HTTPClient.cpp:50
+  return LineRef.consume_front("Content-Length: ") &&
+         !getAsUnsignedInteger(LineRef.trim(), 10, ContentLength);
+}
----------------
labath wrote:
> 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).
Ah, the perfect fit. Thanks!


================
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);
----------------
labath wrote:
> EXPECT_EQ(Handler.ResponseBuffer.Body->getBuffer(), "...");
Thanks! Because the Body is actually a WritableMemoryBuffer, I need an extra namespace qualifier on getBuffer for this to work.

On that note -- do you think I should change the HTTP response body to be a MemoryBuffer (read-only)?



================
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);
----------------
labath wrote:
> What is this supposed to test? This test pretty much only executes code which is defined in the test itself...
I agree and I will remove the mockHTTPClientTest.

Do you think there is any value in the BrokenConnection test? I would be happy to remove this test as well. The only non-test code executed in that one is `Client.perform(Request)` and `Client.get("")` which is effectively a change-detector for the short convenience methods of the Client.


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

https://reviews.llvm.org/D112751