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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:61
+  AUX_STAT = 249
+};
+
----------------
there is enum define as enum SymbolAuxType in xcoff.h, I think you can use it instead of define a new one.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:64
+struct AuxSymbolEnt {
+  AuxSymbolType Type;
+
----------------
please add some comment here " it is for 64bits only"


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:89
+  Optional<uint32_t> StabInfoIndex;
+  Optional<uint16_t> StabSectNum;
+
----------------
I am prefer to add some comments on the data member to indicate the one is for 32bits or 64bits only, or it is for both.
and rearrange the member in the order of common field for both 32bits and 64bits first , only for 32bits, only for the 64bits.   


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:110
+struct ExcpetionAuxEnt : AuxSymbolEnt {
+  Optional<uint32_t> OffsetToExceptionTbl;
+  Optional<uint32_t> SizeOfFunction;
----------------
ExcpetionAuxEnt  is 64bit only and OffsetToExceptionTbl is 8 bytes.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:160
+  Optional<uint8_t> NumberOfAuxEntries;
+  std::vector<std::unique_ptr<AuxSymbolEnt>> AuxEntries;
 };
----------------
Esme wrote:
> Higuoxing wrote:
> > 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<>`.
> I think there is no harm in using `std::unique_ptr<>` since it has less overhead. While wrapping it with `Optional<>` will cause failure in `mapOptional()`.
I think it need std::unique_ptr<> here,  for the AuxEntries is std::vector<> of the pointer to AuxSymbolEnt, without the std::unique_ptr<>, we have to delete all the pointer of AuxSymbolEnt manually otherwise there is memory leak.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113552