[PATCH] D77080: [NFC][XCOFF][AIX] Refactor get/setContainingCsect

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1849
 
-      if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
-        cast<MCSymbolXCOFF>(Sym)->setContainingCsect(
----------------
Can we expect that `hasCsectMCSectionXCOFF` is true for an undefined XCOFF symbol? Do we want to assert that?


================
Comment at: llvm/lib/CodeGen/MachineModuleInfo.cpp:109
   if (Context.getObjectFileInfo()->getTargetTriple().isOSBinFormatXCOFF()) {
     MCSymbol *FnEntryPointSym =
         Context.lookupSymbol("." + Entry.Fn->getName());
----------------
This is unused except by the assert.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:16
+         "Trying to get csect representation of this symbol but none was set.");
+  assert((!getName().equals(getUnqualifiedName()) ||
+          Csect->getCSectType() == XCOFF::XTY_ER) &&
----------------
Just noting that the setter already checks this, which explains why the assert does not need to be written in terms of what the callee is trying to do--it can be written in terms of the internal state of this object.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:18
+          Csect->getCSectType() == XCOFF::XTY_ER) &&
+         "Csect could only get for a symbol represents a csect itself.");
+  return Csect;
----------------
Suggestion:
Symbol does not represent a csect; MCSectionXCOFF that represents the symbol should not be (but is) set.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:23
+void MCSymbolXCOFF::setCsectMCSectionXCOFF(MCSectionXCOFF *C) {
+  assert((!Csect || Csect == C) &&
+         "Trying to set a csect that doesn't match the one that"
----------------
Should also assert that `C` is not null.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:28
+          C->getCSectType() == XCOFF::XTY_ER) &&
+         "Csect could only set for a symbol represents a csect itself.");
+  Csect = C;
----------------
Suggestion:
Symbol does not represent a csect; can only set a MCSectionXCOFF representation for a csect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77080