Discussion:
[PATCH] D37282: clangd: Tolerate additional headers
Ben Jackson via Phabricator via cfe-commits
2017-08-31 21:34:19 UTC
Permalink
puremourning updated this revision to Diff 113476.
puremourning added a comment.

Validate that the duplicate message is printed.


https://reviews.llvm.org/D37282

Files:
clangd/JSONRPCDispatcher.cpp
test/clangd/protocol.test
Ben Jackson via Phabricator via cfe-commits
2017-09-02 17:14:54 UTC
Permalink
puremourning added a comment.

So I think this is ready now. Anything more you need from me?



================
Comment at: clangd/JSONRPCDispatcher.cpp:138
bool &IsDone) {
+ unsigned long long ContentLength = 0;
while (In.good()) {
----------------
Maybe we could move that variable into loop body by splitting the loop body into multiple parts?
Having it outside a loop makes it harder to comprehend the parsing code.
```
while (In.good()) {
// Read first line
//....
llvm::StringRef LineRef = /*...*/;
// Skip comments
if (LineRef.startsWith("#"))
continue;
// Read HTTP headers here, reading multiple lines right inside the loop.
unsigned ContentLength = 0;
// ....
// Read ContentLength bytes of a message here.
std::vector<char> JSON;
//....
}
```
I'll take a look, sure. Something like...
```
while ( the input stream is good )
{
set len to 0
while ( the input stream is good )
{
read a line into header
if the line is a comment, read another line (continue the inner loop !)
if header is Content-Length, store the length in len
otherwise, if header is empty, we're no longer reading headers, break out of the inner loop
}
if the input stream is not good, break out of the loop
if len is 0, reset the outer loop
read len bytes from the stream,
if we read len bytes ok, parse JSON and dispatch to the handlers
}
```
done.


================
Comment at: clangd/JSONRPCDispatcher.cpp:163
+ if (LineRef.consume_front("Content-Length: ")) {
+ llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
+ continue;
----------------
Maybe log an error if `Content-Length` is specified twice?
OK, though I suspect this would only happen in the tests :)


================
Comment at: clangd/JSONRPCDispatcher.cpp:167
+ // It's another header, ignore it.
continue;
+ } else {
----------------
Maybe log the skipped header?
I think this would just be noise for conforming clients which send the (legitimate) Content-Type header. We could of course special case this, but I'm not sure the extra logging would be all that useful.


https://reviews.llvm.org/D37282
Ilya Biryukov via Phabricator via cfe-commits
2017-09-04 10:28:22 UTC
Permalink
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Looks good and ready to land. Thanks for this change.
Do you have commit rights to llvm repo?


https://reviews.llvm.org/D37282
Ilya Biryukov via Phabricator via cfe-commits
2017-09-04 10:33:25 UTC
Permalink
ilya-biryukov added inline comments.


================
Comment at: clangd/JSONRPCDispatcher.cpp:167
+ // It's another header, ignore it.
continue;
+ } else {
----------------
Post by Ben Jackson via Phabricator via cfe-commits
Maybe log the skipped header?
I think this would just be noise for conforming clients which send the (legitimate) Content-Type header. We could of course special case this, but I'm not sure the extra logging would be all that useful.
You're right. Logging `Content-Type` headers doesn't make any sense.


https://reviews.llvm.org/D37282
Ben Jackson via Phabricator via cfe-commits
2017-09-04 11:43:48 UTC
Permalink
puremourning added a comment.
Post by Ilya Biryukov via Phabricator via cfe-commits
Looks good and ready to land. Thanks for this change.
Do you have commit rights to llvm repo?
Thanks for the review!

No I don't have commit rights. Can you land for me? Thanks!


https://reviews.llvm.org/D37282
Phabricator via Phabricator via cfe-commits
2017-09-04 12:29:31 UTC
Permalink
This revision was automatically updated to reflect the committed changes.
Closed by commit rL312483: clangd: Tolerate additional headers (authored by ibiryukov).

Repository:
rL LLVM

https://reviews.llvm.org/D37282

Files:
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/test/clangd/protocol.test
Ilya Biryukov via Phabricator via cfe-commits
2017-09-04 13:33:13 UTC
Permalink
ilya-biryukov added a comment.
Post by Ben Jackson via Phabricator via cfe-commits
No I don't have commit rights. Can you land for me? Thanks!
Done.


Repository:
rL LLVM

https://reviews.llvm.org/D37282

Loading...