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

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:674
+                                            uint8_t MapClass) {
+  W.write<uint32_t>(Lo_32(SecLen)); // x_scnlen
+  W.write<uint32_t>(0);             // x_parmhash
----------------
DiggerLin wrote:
> I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ?   maybe the SectionOrLength from XCOFFObjectFile.h is better.
> I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ?   maybe the SectionOrLength from XCOFFObjectFile.h is better.




================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:674
+                                            uint8_t MapClass) {
+  W.write<uint32_t>(Lo_32(SecLen)); // x_scnlen
+  W.write<uint32_t>(0);             // x_parmhash
----------------
Esme wrote:
> DiggerLin wrote:
> > I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ?   maybe the SectionOrLength from XCOFFObjectFile.h is better.
> > I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ?   maybe the SectionOrLength from XCOFFObjectFile.h is better.
> 
> 
I would prefer the `x_scnlen` one because it is easier for the developer to refer to the documentation, and this comment style references `ELFObjectWriter.cpp`.


================
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
----------------
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:756
 
-    W.write<uint32_t>(Sec->Size);
-    W.write<uint32_t>(Sec->FileOffsetToData);
-    W.write<uint32_t>(Sec->FileOffsetToRelocations);
-
-    // Line number pointer. Not supported yet.
-    W.write<uint32_t>(0);
-
-    W.write<uint16_t>(Sec->RelocationCount);
-
-    // Line number counts. Not supported yet.
-    W.write<uint16_t>(0);
+    W.write<uint32_t>(Sec->Size);                    // s_size
+    W.write<uint32_t>(Sec->FileOffsetToData);        // s_scnptr
----------------
DiggerLin wrote:
> I do not think we need the s_size here , Sec->Size can self-explain.
> ditto for the following.
Removed `// s_size` and `// s_flags`, and kept other comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120858