[PATCH] D115418: [dwarf][NFC] Move expandBundle() to MachO.h

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:663-664
+        llvm::append_range(Objects, *DsymObjectsOrErr);
+    } else
+      error(DsymObjectsOrErr.takeError());
   }
----------------
My reading of the relatively recently updated coding standards is that if part of an if statement has braces, the corresponding else clause(s) also need braces. Applies in the llvm-gsymutil case too.

Looking at the `findDsymObjectMembers`, it looks like there is still missing context, in the event of a filesystem error, so you'd end up with a message that says something like: `error: foo.dsym: access denied`, which doesn't explain the what was happening at the time. I'm not sure the function itself should necessarily add this context, but could be persuaded it should. My thinking was that we should use the `Prefix` taking `error` method, and add the context that way, e.g. `error("whilst loading objects from DSYM bundle", DsymObjectsOrErr.takeError())`. I believe this would result in a message like: `error: whilst loading objects from DSYM bundle: foo.dsym: access denied`.

Thoughts? Downside is that the "no objects" case will end up repeating the context a bit more than is strictly necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115418