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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:73-74
+Expected<HTTPResponseBuffer> HTTPClient::perform(const HTTPRequest &Request) {
+  BufferedHTTPResponseHandler Handler;
+  if (Error Err = perform(Request, Handler))
+    return std::move(Err);
----------------
Looks like this API doesn't require the `HTTPResponseHandler` to have a virtual dtor - if these objects will usually be owned directly, rather than indirectly through a unique_ptr of the base type, etc, then the dtor can be non-virtual (but the base type will have to make the dtor `protected` and concrete derived classes would need to be marked `final`)


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:75
+
+  EXPECT_THAT_ERROR(Handler.handleHeaderLine("Content-Length: \0\0\0"),
+                    Succeeded());
----------------
noajshu wrote:
> dblaikie wrote:
> > This probably doesn't test what it's intended to test - the string will terminate at the first '\0' - if you want to test embedded null characters, you'd need to specify an explicit length when creating the StringRef explicitly, rather than relying on StringRef(const char*) conversion.
> Thanks for pointing this out! I have added a fix using C++14 string literals. If you prefer I can also change this to use a hardcoded string length instead.
Oh, great! 

If the string data doesn't need to outlive the `handleHeaderLine` call, you could roll it in:
```
...handleHeaderLine("Content-Length: \0\0\0"s),
...
```

Rather than having a standalone named variable. Though if you prefer the named variable to document the code intent, that's totally fair too!


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

https://reviews.llvm.org/D112751