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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/ELF/symbol.yaml:16
+
+!ELF
+FileHeader:
----------------
jhenderson wrote:
> 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.
You actually can add documents, you just need to be sure that 2nd and other document has `--- !ELF`.
For the first one it is not really important. I did it though.


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:1
+## Check we can set different values and sizes for symbols.
+
----------------
jhenderson wrote:
> 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).
I did it for consistency. Currently we have the following list of tests for symbols:

```
symbol-stother.yaml
symbol-type.yaml
symbol-binding.yaml
symbol-index.yaml
symbol-index-invalid.yaml
symbol-name.yaml
symbol-visibility.yaml
```

Perhaps, we should merge them into a single test in a follow-up.


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:3
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --symbols %t | FileCheck %s
----------------
jhenderson wrote:
> Perhaps worth a comment saying something like "show the default size and value is 0".
The symbol to "show the default size and value is 0" is `aaa`, I have the comment for it below.
This particular call and check lines are not not testing anything valuable (used to bypass
the header and the null symbol).


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

https://reviews.llvm.org/D93010