[PATCH] D113552: [yaml2obj][XCOFF] parsing auxiliary symbols.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:238
+  for (XCOFFYAML::Symbol &YamlSym : Obj.Symbols) {
+    uint8_t AuxCount = YamlSym.AuxEntries.size();
+    if (YamlSym.NumberOfAuxEntries && *YamlSym.NumberOfAuxEntries < AuxCount) {
----------------
You could store this a `uint32_t` to save the later cast, if you want.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:241
+      ErrHandler("specified NumberOfAuxEntries " +
+                 Twine((uint32_t)*YamlSym.NumberOfAuxEntries) +
+                 " is less than the actual number "
----------------
I think we prefer static_cast


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/aux-symbols-full-contents.yaml:1
+## Test that we can explicitly specify all fields for auxiliary symbols.
+
----------------
Esme wrote:
> jhenderson wrote:
> > Rather than needing to duplicate the YAML between this and the defaults test case, you could consider implementing support for (if needed) and using the special `[[MACRO=<none>]]`  syntax in the YAML. For ELF, this is equivalent to omitting the field, if no value is specified for it, which allows you to test default behaviour, using the same YAML as non-default behaviour.
> The `[[MACRO=<none>]]` syntax will work well to reduce duplication when the same field is used for multiple assignments. But in this test, most of the fields only need to be tested once. 
> Besides, 32-bit and 64-bit may have different fields for the same aux symbol type, so I have to use two YAMLs. I don't think using [[MACRO=<none>]] here will simplify the test.
> And I think it would be easier to review if I separate the default and non-default behaviors?
You've got two general test cases: the default and the non default. In both cases, you need a fair amount of basic scaffolding for the YAML to work (i.e. symbol details etc). In the default case, you can omit some fields, for the aux entries, but not in the full contents cases. I'm suggesting you merge the default and non-default YAML documents, not the 32/64 cases, so that the basic scaffolding isn't duplicated across the two files.

I don't think it's any easier to review separating default and non-default cases, personally. You'll see this pattern a lot in the ELF tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113552