[PATCH] D114415: [llvm] [Support] (WIP) Add HTTP Client Support library.

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/include/llvm/Support/HTTPServer.h:1
+#ifndef LLVM_SUPPORT_HTTP_SERVER_H
+#define LLVM_SUPPORT_HTTP_SERVER_H
----------------
This file is missing LLVM header.


================
Comment at: llvm/lib/Support/HTTPServer.cpp:1
+#include "llvm/Support/HTTPServer.h"
+#include "llvm/ADT/StringExtras.h"
----------------
This file is missing LLVM header.


================
Comment at: llvm/lib/Support/HTTPServer.cpp:19
+HTTPServer::HTTPServer(std::function<HTTPResponse(StringRef)> RequestHandler) {
+  Server.Get(R"(/(.*))",
+             [&](const httplib::Request &Request, httplib::Response &Response) {
----------------
I wonder if we should perhaps move this to a separate method (like `get`) and take the path as an argument so we can potentially support multiple handlers.


================
Comment at: llvm/lib/Support/HTTPServer.cpp:23
+               UrlPath = std::string(Request.matches[1]);
+               HTTPResponse Resp = RequestHandler(UrlPath);
+               Response.set_content(Resp.Body.data(), Resp.ContentType.data());
----------------
Having both `Response` and `Resp` is confusing, can we use a different name here?


================
Comment at: llvm/lib/Support/HTTPServer.cpp:57
+    Server.listen_after_bind();
+  return Error::success();
+}
----------------
Calling `listen` on an unbound server should probably return an error rather than success.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114415