[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/yaml2obj/ELF/symbol-value-size.yaml:10
+
+!ELF
+FileHeader:
----------------
Perhaps `---`


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:1
+## Check we can set different values and sizes for symbols.
+
----------------
grimar wrote:
> 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.
I think it would make sense to either have all fields tested in a single file, or all in separate files. I don't think the hex nature of these ones is sufficient reason for these two to be diffferent. If doing them all in a single file, you could do everything using decimal, and then show that hex can be used too in an additional single case at the end, I guess.


================
Comment at: llvm/test/tools/yaml2obj/ELF/symbol-value-size.yaml:3
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --symbols %t | FileCheck %s
----------------
grimar wrote:
> 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).
Perhaps we don't need the header check at all actually? As it is, it looks like it's verifying something important, but the null entry and symbol count should be covered elsewhere, I reckon.


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

https://reviews.llvm.org/D93010