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

[Xing GUO via Phabricator via llvm-commits]

Higuoxing added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:160
+  Optional<uint8_t> NumberOfAuxEntries;
+  std::vector<std::unique_ptr<AuxSymbolEnt>> AuxEntries;
 };
----------------
Question 1: Why we need `std::unique_ptr<>` here?

Question 2: It looks that `AuxEntries` is optional in the symbol entry. Can we wrap it with `Optional<>`.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:205
 
+  // Check if the file name in the File Auxiliary Entry should be addes to the
+  // string table.
----------------
Nit.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:207
+  // string table.
+  for (XCOFFYAML::Symbol &YamlSym : Obj.Symbols) {
+    for (const std::unique_ptr<XCOFFYAML::AuxSymbolEnt> &AuxSym :
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:238-239
+    uint8_t AuxCount = YamlSym.AuxEntries.size();
+    if (YamlSym.NumberOfAuxEntries && *YamlSym.NumberOfAuxEntries < AuxCount)
+    {
+      ErrHandler("specified NumberOfAuxEntries is less than the actual number "
----------------
Nit: coding style.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:240
+    {
+      ErrHandler("specified NumberOfAuxEntries is less than the actual number "
+                 "of auxiliary entries");
----------------
It would be great to print the values of `NumberOfAuxEntries` and `AuxCount` in the error message.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:221-222
+  } else {
+    IO.mapOptional("LineNum_Hi", AuxSym.LineNum_Hi);
+    IO.mapOptional("LineNum_Lo", AuxSym.LineNum_Lo);
+  }
----------------
Why the names of these two entries are not consistent with other fields (e.g., `SectionOrLengthLo`, `SectionOrLengthHi`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113552