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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:45
+                                     OffsetBuffer *Buf) {
+  assert(Size == 1);
+
----------------
We usually also try to provide a message explaining what went wrong: `assert(Size == 1 && "...")`.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:50-52
+#define HEADER_PREFIX "Content-Length: "
+#define HEADER_PREFIX_LEN 16
+  if (StringRef(Contents, HEADER_PREFIX_LEN) == HEADER_PREFIX) {
----------------
I'd avoid the macros which we don't usually use in C++ and instead use the `StringRef` methods, so you can do something like:

```
StringRef Header(Contents, NMemb);
if (Header.starts_with("Content-Length: ")) {
  StringRef Length = Header.substr(16);
  size_t ContentLength;
  if (!Length.getAsInteger(10, ContentLength)) {
    ...
  }
}
```

We should also make the code more resilient to handle potential issues, for example could the header look like `Content-Length    :     N` or is it guaranteed to always be `Content-Length: N`?


================
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(
----------------
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).


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

https://reviews.llvm.org/D111252