[PATCH] D111252: [llvm] [Support] [Debuginfo] Add http and debuginfod client libraries and llvm-debuginfod-find tool

[Noah Shutty via Phabricator via llvm-commits]

noajshu marked an inline comment as done.
noajshu added inline comments.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:52
+#define HEADER_PREFIX_LEN 16
+  if (StringRef(Contents, HEADER_PREFIX_LEN) == HEADER_PREFIX) {
+    size_t ContentLength = std::stoll(
----------------
noajshu wrote:
> phosek wrote:
> > If the header doesn't contain `Content-Length`, would we fail in `writeMemoryCallback` because we haven't allocated the buffer? I think the should be more resilient to handle that situation somehow (even if just by returning an error).
> Instead of asserting the buffer has been allocated, I changed it to terminate the download and return an Error. However I agree we ought to be more resilient in this case especially since Transfer-Encoding is a valid alternative to Content-Length.
> Perhaps we could fall back to repeated re-allocations in the case there is no Content-Length header?
The latest diff handles a missing `Content-Length` by falling back to a `std::vector` buffer. This has the disadvantage of a `resize` on each write callback and an additional allocation + copy to convert to the returned `MemoryBuffer`. In my test it downloads ~24x slower without the Content-Length header.


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

https://reviews.llvm.org/D111252