[PATCH] D120858: [NFC][XCOFF] Refactor and format XCOFFObjectWriter.cpp.

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:274
+  void writeSymbolAuxCsectEntry(uint64_t, uint8_t, uint8_t);
+  void writeSymbolEntry(StringRef, uint64_t, int16_t, uint16_t, uint8_t,
+                        uint8_t);
----------------
using writeSymbolAuxCsectEntry(uint64_t SecLen, uint8_t Type, uint8_t MapClass) is easy to understand the interface than  writeSymbolAuxCsectEntry(uint64_t, uint8_t, uint8_t);
same as 
writeSymbolAuxDwarfEntry
writeSymbolEntry


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:652
 
-void XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel(
-    const Symbol &SymbolRef, const XCOFFSection &CSectionRef,
-    int16_t SectionIndex, uint64_t SymbolOffset) {
-  // Name or Zeros and string table offset
-  writeSymbolName(SymbolRef.getSymbolTableName());
-  assert(SymbolOffset <= UINT32_MAX - CSectionRef.Address &&
-         "Symbol address overflows.");
-  W.write<uint32_t>(CSectionRef.Address + SymbolOffset);
-  W.write<int16_t>(SectionIndex);
+void XCOFFObjectWriter::writeSymbolEntry(StringRef SymbolName, uint64_t Value,
+                                         int16_t SecIdx, uint16_t Type,
----------------
uint64_t Value--> uint32_t Value


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:655
+                                         uint8_t StorageClass,
+                                         uint8_t AuxCnt = 1) {
+  writeSymbolName(SymbolName); // n_name, n_zeros, n_offset
----------------
maybe better to change the AuxCnt to NumberOfAuxEntries ?
change SecIdx to  SectionNumber 
etc , 
I think the parameter name should be consistent with XCOFFObject.h.

same as function  
writeSymbolAuxDwarfEntry and writeSymbolAuxCsectEntry


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:671
+
+void XCOFFObjectWriter::writeSymbolAuxCsectEntry(uint64_t SecLen, uint8_t Type,
+                                                 uint8_t MapClass) {
----------------
the function looks only support 32bit AuxCsect now .  "uint64_t SecLen" should be "uint32_t SecLen"? if we change to "uint32_t SecLen" 

W.write<uint32_t>(Lo_32(SecLen)); // x_scnlen  

also need to be changed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:682
+
+void XCOFFObjectWriter::writeSymbolAuxDwarfEntry(uint64_t SecLen,
+                                                 uint64_t RelCnt = 0) {
----------------
"uint64_t SecLen" should be "uint32_t SecLen" ? and  "uint64_t RelCnt" should be "uint32_t RelCnt"


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:708
 
-  // n_name, n_zeros, n_offset
-  writeSymbolName(DwarfSectionRef.getSymbolTableName());
-  // n_value
-  W.write<uint32_t>(0);
-  // n_scnum
-  W.write<int16_t>(SectionIndex);
-  // n_type
-  W.write<uint16_t>(0);
-  // n_sclass
-  W.write<uint8_t>(XCOFF::C_DWARF);
-  // Always 1 aux entry for now.
-  W.write<uint8_t>(1);
-
-  // Now output the auxiliary entry.
-  // x_scnlen
-  W.write<uint32_t>(DwarfSectionRef.Size);
-  // Reserved
-  W.write<uint32_t>(0);
-  // x_nreloc. Set to 0 for now.
-  W.write<uint32_t>(0);
-  // Reserved
-  W.write<uint32_t>(0);
-  // Reserved
-  W.write<uint16_t>(0);
+  writeSymbolEntry(DwarfSectionRef.getSymbolTableName(), 0, SectionIndex, 0,
+                   XCOFF::C_DWARF);
----------------
change "DwarfSectionRef.getSymbolTableName(), 0, SectionIndex, 0,"
-->
DwarfSectionRef.getSymbolTableName(), /*Value=*/0, SectionIndex,/*Type=*/ 0,

adding comment will make the value "0" easy to understand. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:718
+  writeSymbolEntry(CSectionRef.getSymbolTableName(), CSectionRef.Address,
+                   SectionIndex, 0, StorageClass);
+
----------------
add a comment for value 0,  ditto in the following value zero or 1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120858