This patch implements a small HTTP client library consisting primarily of the HTTPRequest, HTTPResponseHandler, and BufferedHTTPResponseHandler classes. Unit tests of the HTTPResponseHandler and BufferedHTTPResponseHandler are included.
Ah, the perfect fit. Thanks!
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.
This looks good to me now, modulo some details in inline comments.
This block looks weird -- I'd just make a separate test for that.
I don't see a need for that -- I can see how the caller might want to modify the buffer for its own purposes. and he can always convert it to a read-only buffer if he does not (the opposite is harder).
Yeah, I'd probably remove them, or at least significantly simplify them. If you want to test that the virtual perform is getting called, then all you need to do is mock that method -- you don't really need to provide a fake implementation of that method, and everything that gets called by it...
I think this is as good as it can get. What you do after that -- I don't know.
It wasn't clear to me whether you forgot about the mockHTTPClientBrokenConnectionTest part, or chose not to do that, but I added an additional comment to describe how I would do this.
MOCK_METHOD2(perform, Error(HTTPRequest, HTTPResponseHandler)), and then just set expectations in the test. The entire MockHTTPResponseHandler business can go away.
Thanks Pavel! I decided to remove the mock client tests, because they only hit the few-line convenience methods. I'm happy to add them, I'm just unsure how to avoid making HTTPClient::perform(Request, Handler) virtual. In any case they will be hit in subsequent testing after a working client implementation (i.e. Curl in D112753) is added.
Thanks! It seems the only library code we hit with these tests is the small convenience methods perform(Request) and get(Url). Is it worth making the perform method virtual to be able to create the mock class?
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?
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)
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.
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)
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.
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.
Skip const here
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)
Skip top-level "const" here - it's out of character for LLVM code.
skip const here
Use = default; here, perhaps?
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.
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.
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)
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!