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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:683
+void XCOFFObjectWriter::writeSymbolAuxDwarfEntry(uint64_t SecLen,
+                                                 uint64_t RelCnt = 0) {
+  W.write<uint32_t>(SecLen); // x_scnlen
----------------
Default arguments are usually specified in the declaration, not definition. Same elsewhere.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:690
+
+void XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel(
+    const Symbol &SymbolRef, const XCOFFSection &CSectionRef,
----------------
For consistency with other names.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:812
   // FIXME: support 64-bit C_FILE symbol.
-  //
-  // n_name. The n_name of a C_FILE symbol is the source filename when no
-  // auxiliary entries are present. The source filename is alternatively
-  // provided by an auxiliary entry, in which case the n_name of the C_FILE
-  // symbol is `.file`.
+  // The n_name of a C_FILE symbol is the source filename when no auxiliary
+  // entries are present. The source filename is alternatively provided by an
----------------
Ditto the line below.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:272-275
+  void writeAuxSymbolDwarf(uint64_t, uint64_t);
+  void writeAuxSymbolCsect(uint64_t, uint8_t, uint8_t);
+  void writeSymbolBody(StringRef, uint64_t, int16_t, uint16_t, uint8_t,
+                       uint8_t);
----------------
Please provide the parameter names here.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:726
 void XCOFFObjectWriter::writeFileHeader() {
-  // Magic.
-  W.write<uint16_t>(0x01df);
-  // Number of sections.
-  W.write<uint16_t>(SectionCount);
-  // Timestamp field. For reproducible output we write a 0, which represents no
-  // timestamp.
-  W.write<int32_t>(0);
-  // Byte Offset to the start of the symbol table.
-  W.write<uint32_t>(SymbolTableOffset);
-  // Number of entries in the symbol table.
-  W.write<int32_t>(SymbolTableEntryCount);
-  // Size of the optional header.
-  W.write<uint16_t>(0);
-  // Flags.
-  W.write<uint16_t>(0);
+  W.write<uint16_t>(is64Bit() ? XCOFF::XCOFF64 : XCOFF::XCOFF32);
+  W.write<uint16_t>(SectionCount);         // f_nscns
----------------
Esme wrote:
> DiggerLin wrote:
> > since the header is for 32bit. 
> >   W.write<uint32_t>(SymbolTableOffset);    // f_symptr
> >   W.write<int32_t>(SymbolTableEntryCount); // f_nsyms
> > 
> > we should modify the code 
> > as 
> > assert(!is64Bit() && "not support 64 bit for writing file header.");
> > W.write<uint16_t>(XCOFF::XCOFF32);
> Thanks!
> Well, I don't think we need assertions here, because we already blocked 64-bit mode with another assertion before calling this function.
I'd have the assertion: in the future, you may not call this function from the same place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120858