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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Couple of minor API design issues, but haven't dug in much.

@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?



================
Comment at: llvm/lib/Support/HTTPClient.cpp:25
+
+HTTPRequest::HTTPRequest(const Twine &Url) { this->Url = Url.str(); }
+
----------------
Twine's mostly meant for when a string value may only be conditionally manifest (such as in a logging library where some dynamic log level would dictate whether the value is logged) - if the string is unconditionally manifest, Twine probably isn't the right tool to use here.

You could pass a SmallString by value & move it into the Url, but since it's a smallstring, it has a big sizeof and might not be the most efficient thing to move around. Alternatively just pass a StringRef.

(similar feedback up the stack/for other uses of Twine)


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:64-71
+  // Confirm llvm::to_integer behaves as expected.
+  size_t ContentLength;
+  EXPECT_EQ(llvm::to_integer("fff", ContentLength, 10), false);
+  EXPECT_EQ(llvm::to_integer("", ContentLength, 10), false);
+  EXPECT_EQ(llvm::to_integer("-1", ContentLength, 10), false);
+  EXPECT_EQ(llvm::to_integer("0111", ContentLength, 10), true);
+  EXPECT_EQ(llvm::to_integer("0", ContentLength, 10), true);
----------------
If this code isn't touching/implementing to_integer, it's probably not necessary/appropriate to re-test it here, should just rely on its existing definition/documentation/testing elsewhere.


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:85
+  EXPECT_THAT_ERROR(
+      Handler.handleHeaderLine(StringRef("Content-Length: \0\0\0", 19)),
+      Succeeded());
----------------
Perhaps avoid the hardcoded 19 somehow? Like you could declare the string as a named char array, then use sizeof to get the length (-1 for the implicit null terminator, if that's not meant to be included)


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

https://reviews.llvm.org/D112751