[PATCH] D93010: [yaml2obj/obj2yaml] - Make Value/Size fields of Symbol optional.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/ELF/symbol.yaml:1
+## This is a test case to check how tool dumps symbols.
+
----------------



================
Comment at: llvm/test/tools/obj2yaml/ELF/symbol.yaml:16
+
+!ELF
+FileHeader:
----------------
That's needed or you won't be able to add more documents in the future to the test file, I've discovered. Same below.


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:1
+## Check we can set different values and sizes for symbols.
+
----------------
I'm slightly concerned that this test name and purpose is a bit restrictive. Why doesn't it apply to other symbol fields, like type, other or section index? (I'm not saying those tests need adding in this change, but it feels like there's no special reason why value and size are in one test and the others aren't in the same one).


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:3
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --symbols %t | FileCheck %s
----------------
Perhaps worth a comment saying something like "show the default size and value is 0".


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:16-17
+Symbols:
+## No "Size" or "Value" keys were set. Check it is the
+## same as if we would set them to 0.
+  - Name:  aaa
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:30
+# CHECK-NEXT: 3: 0000000000000000 0    NOTYPE  LOCAL  DEFAULT   UND ccc
+## Both "Size" and "Value" are explicitly set to an arbitraty value.
+## Here we use UINT64_MAX to check this boundary case.
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:36
+# CHECK-NEXT: 4: ffffffffffffffff -1   NOTYPE  LOCAL  DEFAULT   UND ddd
+## The same as previous, but decimal values were used.
+  - Name:  eee
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93010