[PATCH] D135117: [objdump] Support finding --source via --dsym files

[Jim Radford via Phabricator via llvm-commits]

radford marked 5 inline comments as done.
radford added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/MachO/disassemble-source-dsym.test:3
+# RUN: dsymutil -f    %p/../../dsymutil/Inputs/basic.macho.x86_64 -o %t1.dSYM         -oso-prepend-path=%p/../../dsymutil
+# RUN: llvm-objdump < %p/../../dsymutil/Inputs/basic.macho.x86_64 - --source  --dsym=%t1.dSYM  --prefix=%p/../../dsymutil| \
+# RUN:   FileCheck --check-prefix=SOURCE %s
----------------
MaskRay wrote:
> Add a comment if this intentionally tests reading from stdin?
I hope the explicit - is enough to make it clear this is intentional.


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:7400
+  if (DSYMFile.empty()) {
+    Twine FilenameDSYM = Filename + ".dSYM";
+    if (llvm::sys::fs::is_directory(FilenameDSYM)) {
----------------
MaskRay wrote:
> https://llvm.org/docs/ProgrammersManual.html#the-twine-class
> 
> Avoid using Twine as a local variable.
I don't see any reference escaping here, AFAICT, but I'm new to llvm's string types.  Could you offer a suggestion?


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:7484
+      WithColor::error(errs(), "llvm-objdump")
+          << DSYMPath << " is not a Mach-O or Universal file type.\n";
+      return nullptr;
----------------
MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
> 
> The convention is omit the trailing period.
These messages are referenced in unrelated tests whose changes would only add noise here.   Also, changing just these would make them inconsistent with the rest of the file. :(


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

https://reviews.llvm.org/D135117