[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/yaml2obj/ELF/symbol-value-size.yaml:1
+## Check we can set different values and sizes for symbols.
+
----------------
grimar wrote:
> 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.
Note that value and size are the only `llvm::yaml::Hex64` values of a symbol.
And the logic of handling them is very similar. It is probably reasonable to test them in a single test/subtest.


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

https://reviews.llvm.org/D93010