[PATCH] D66969: Output XCOFF object text section

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

I might have more comments later, but I'm posting what I have currently.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:325
 
+bool XCOFFObjectWriter::isNameInStringTable(const StringRef &SymbolName) {
+  return SymbolName.size() > XCOFF::NameSize;
----------------
This really sounds like the name is already in the string table.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:351
+  // table entries for a detailed description. Bits 0-3 is for symbol
+  // visibility. Bit 10 (0x0020) can be set if the symbol is a function in the
+  // old XCOFF32 interpretation. Otherwise bit 10 should be 0. Since we don't
----------------
The old comment is fine here. Please switch back to the shorter version. This is a part of the documentation that requires careful reading in context (and is a bad idea to quote portions from).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:356
+  // TODO FIXME How to assert a symbol's visibilty is default?
+  // TODO We aren't supporting the old XCOFF32 interpretation, but rather
+  // setting the bit (which is optional in the new interpretation) when
----------------
The second TODO should say:
```
// TODO Set the function indicator (bit 10, 0x0020) for functions
// when debugging is enabled.
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:391
+  // table entries for a detailed description. Bits 0-3 is for symbol
+  // visibility. Bit 10 (0x0020) can be set if the symbol is a function in the
+  // old XCOFF32 interpretation. Otherwise bit 10 should be 0. Since we don't
----------------
Use the shorter version of the comment here too,


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:463
 
-void XCOFFObjectWriter::writeSymbolTable() {
-  assert(ProgramCodeCsects.size() == 0 && ".text csects not handled yet.");
+void XCOFFObjectWriter::writeSymbolTable(const llvm::MCAsmLayout &Layout) {
+  // Print out symbol table for the program code.
----------------
I'm not seeing the change. `llvm::` still here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969