[PATCH] D107421: [yaml2obj][XCOFF] Customize the string table.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm going to come back to looking at this another day, as I'm out of time now.

I do wonder if it would make sense to model the behaviour of symbol names and the string table on the existing ELF behaviour, i.e. what happens when a name isn't present etc. Perhaps worth looking at ELF and mirroring the approach?



================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:63
+struct StringTable {
+  bool isSuppress;
+  uint32_t Length;
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:38
     Is64Bit = Obj.Header.Magic == (llvm::yaml::Hex16)XCOFF::XCOFF64;
+    isStrTblSuppress = Obj.StrTbl.isSuppress;
   }
----------------
Seems like this is an unnecessary member variable? Just do `Obj.StrTbl.isSuppress` everywhere you need it.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:148
+  StrTbl.clear();
+  // Set the string table length field with the custom value.
+  if (Obj.StrTbl.Length)
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:152
+
+  for (const StringRef &StringEnt : Obj.StrTbl.Strings)
+    StrTbl.add(StringEnt);
----------------
Don't use `const &` for `StringRef` types - the type is lightweight and designed to be copied.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:207
+  
+  // Customize the string table.
+  initStringTable();
----------------
Customizing is one aspect of initialisation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421