[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/include/llvm/Support/HTTPClient.h:21-27
+namespace llvm {
+struct HTTPResponse {
+  long Code = 0;
+  std::string Body;
+};
+Expected<HTTPResponse> httpGet(const Twine &Url);
+} // end namespace llvm
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:29-30
+
+using namespace llvm;
+using namespace sys::fs;
+
----------------



================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:38
+    std::function<void(size_t, std::unique_ptr<MemoryBuffer>)> AddBuffer) {
+
+  LLVM_DEBUG(dbgs() << "fetching info, debuginfod urls size = "
----------------
Nit: no empty line.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:42-51
+  if (Type == DebuginfodAssetType::Executable) {
+    Suffix = "executable";
+  } else if (Type == DebuginfodAssetType::Debuginfo) {
+    Suffix = "debuginfo";
+  } else if (Type == DebuginfodAssetType::Source) {
+    // Description is the absolute source path
+    Suffix = "source" + Description.str();
----------------
This could be a `switch`.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:80
+  for (auto &ServerUrl : DebuginfodUrls) {
+
+    SmallString<64> AssetUrl;
----------------
Nit: no empty line.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:104
+
+#undef DEBUG_TYPE
----------------



================
Comment at: llvm/lib/Support/HTTPClient.cpp:24
+  size_t realsize = size * nmemb;
+  struct MemoryStruct *mem = (struct MemoryStruct *)userp;
+
----------------
phosek wrote:
> This code should be using C++ casts, not C casts.
This is still not addressed.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:44
+
+  struct MemoryStruct chunk;
+
----------------
phosek wrote:
> I'd use a `std::vector` here and resize it inside the callback as needed. That's not only more ergonomic, but also automatically releases memory.
This is still not addressed.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:52
+  if (!Curl) {
+    return createStringError(errc::io_error, "http library error");
+  }
----------------
phosek wrote:
> This leaks `chunk.memory`.
This is still not addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111252