[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 4 inline comments as done.
noajshu added inline comments.


================
Comment at: llvm/lib/Support/CMakeLists.txt:79
+if (LLVM_ENABLE_CURL)
+  set(system_libs ${system_libs} ${CURL_LIBRARIES})
+endif()
----------------
labath wrote:
> Who sets `CURL_LIBRARIES` ?
When `LLVM_ENABLE_CURL` is on, `llvm/cmake/config-ix.cmake` calls `find_package(CURL)` which [[ https://cmake.org/cmake/help/latest/module/FindCURL.html | sets the ]] `CURL_LIBRARIES`. This was added in D111238.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:15-23
+#include "llvm/Support/HTTPClient.h"
+
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
----------------
labath wrote:
> This isn't super important, but the best way to ensure you follow the [[ https://llvm.org/docs/CodingStandards.html#include-style | rules for include ordering ]], is to put them in a single block (with no empty lines), as then clang-format will order them for you.
That's a great tip, thanks!


================
Comment at: llvm/lib/Support/HTTPClient.cpp:34
+  std::unique_ptr<WritableMemoryBuffer> Buffer;
+};
+
----------------
labath wrote:
> The anonymous namespace should end here, and the rest should be static functions, per <https://llvm.org/docs/CodingStandards.html#anonymous-namespaces>
Thanks!


================
Comment at: llvm/tools/llvm-debuginfod/CMakeLists.txt:9
+    )
+  set_property(TARGET llvm-debuginfod-find PROPERTY LLVM_SYSTEM_LIBS ${imported_libs})
+  if(LLVM_INSTALL_BINUTILS_SYMLINKS)
----------------
labath wrote:
> What's the purpose of this?
Good catch, this is no longer required.


================
Comment at: llvm/tools/llvm-debuginfod/llvm-debuginfod-find.cpp:78
+      !sys::path::cache_directory(CacheDirectoryPath))
+    CacheDirectoryPath = ".";
+
----------------
labath wrote:
> When does the current directory fallback kick in? Is it actually useful? Should you exit instead?
The fallback would kick in when `cache_directory` comes up empty-handed. This can happen on linux if neither `$XDG_CACHE_HOME` nor `$HOME` are in the environment. I would have no problem with removing the fallback and failing in this case as the user can always specify the current directory using `--cache-dir` anyways.


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:15
+
+TEST(HTTPClientTests, invalidUrlTest) {
+  std::string invalidUrl = "llvm is fun";
----------------
labath wrote:
> phosek wrote:
> > Ditto for this one.
> This would be better done as `EXPECT_THAT_EXPECTED(httpGet(...), Failed<StringError>())` or even `FailedWithMessage("whatever")`, though I agree that this is not very useful.
> 
> I think the interesting question is whether we're ok with not having any coverage for the lower level apis, and I'm not really sure what the answer to that is.
Thanks, updated!
I agree with the comments that these unit tests are not adding much value. One option until we have a server in llvm would be to GET http://llvm.org, although that does require an internet connection. By the lower level APIs, are you referring to the static callback functions used to implement `httpGet`, or to the CURL library itself?


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

https://reviews.llvm.org/D111252