[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for the delay in looking at this further. I haven't had a chance to look at most of this again, but here are a few smaller comments.



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:65-68
+  // -2 Specifies N_DEBUG, a special symbolic debugging symbol.
+  // -1 Specifies N_ABS, an absolute symbol. The symbol has a value but is not
+  // relocatable.
+  //  0 Specifies N_UNDEF, an undefined external symbol.
----------------
It would probably be good to use some enum values or named constants for these, e.g. something like:
```
namespace xcoff {
enum {
  N_DEBUG = -2,
  N_ABS = -1,
  N_UNDEF = 0
}
}
```
This is similar to how special section indexes are handled for ELF.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:74
+  uint64_t CurrentOffset =
+      sizeof(XCOFF::FileHeader32) /* TODO + auxiliaryHeaderSize()*/ +
+      InitSections.size() * sizeof(XCOFF::SectionHeader32);
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:78
+
+  for (uint16_t I = 0; I < InitSections.size(); I++) {
+    if (CurrentOffset > MaxRawDataSize) {
----------------
LLVM style guide is to precalculate the number of sections, if the number cannot change within the loop, as per the suggestion inline. Also, prefer preincrement to postincrement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505