[PATCH] D37282: clangd: Tolerate additional headers

[Ben Jackson via Phabricator via cfe-commits]

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