[PATCH] D88468: [llvm-readobj] Fix error when printing out stabs symbols of type N_OSO

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/Inputs/stabs.obj.macho-x86_64.yaml:1
+--- !mach-o
+FileHeader:
----------------
Do you need a separate input file? I think you can inline it to the test file.
(See how tests in llvm-readobj/ELF do).


================
Comment at: llvm/test/tools/llvm-readobj/MachO/Inputs/stabs.obj.macho-x86_64.yaml:250
+    - ''
+...
----------------
I am not an expert in MachO, but It feels that this YAML contains excessive symbols, sections, strings etc.
The best practice is to have tests as little as possible to improve readability. Can this one be reduced?


================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-macho.test:1
+# RUN: yaml2obj %p/Inputs/stabs.obj.macho-x86_64.yaml -o %t
+# RUN: llvm-readobj --syms %t | FileCheck %s
----------------
All of our llvm-readobj tests for ELF normally have a header with a description that explains
what is tested. I'd recommend doing this for MachO too.

Also, since your new test is in the `MachO` folder, you probably don't need a `-macho` suffix
in its name, it is excessive.


================
Comment at: llvm/test/tools/llvm-readobj/MachO/stabs-macho.test:3
+# RUN: llvm-readobj --syms %t | FileCheck %s
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
----------------
Please separate `CHECK` line from `RUN` line with an empty line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88468