[PATCH] D132887: [objdump] [debuginfod] Fetch for very-stripped binaries.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1865
+  OwningBinary<Binary> FetchedBinary;
+  if (Obj->sections().empty() || Obj->symbols().empty()) {
+    if (Optional<OwningBinary<Binary>> FetchedBinaryOpt =
----------------
There's a lot of if clauses in this new area of code, so I'd expect to see a corresponding number of new test cases, but there's only one. Please make sure there's a test case that exercises each of the different possible code paths.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1984-1986
+      if (ObjectFile *FetchedObj =
+              dyn_cast<ObjectFile>(DebugBinaryOpt->getBinary());
+          FetchedObj->hasDebugInfo()) {
----------------
Might want to hold off on this usage here: there's an active discussion on whether we a) should allow it (due to potential footguns), and b) what the style of the C++17 if with initializer should be. See https://discourse.llvm.org/t/rfc-code-style-of-initializer-in-if-statement/65357/1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132887