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()) {
----------------
puremourning wrote:
> ilya-biryukov wrote:
> > 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.
> > Rewriting to something like this would arguably make it easier to read:
> > ```
> > 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;
----------------
ilya-biryukov wrote:
> 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 {
----------------
ilya-biryukov wrote:
> 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
