[PATCH] D152945: [SystemZ][z/OS] Implement executePostLayoutBinding for GOFFWriter

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCAssembler.h:489
 
+  std::pair<std::string, std::string> getCsectNames() { return CsectNames; }
+
----------------
I think we can return a reference here to avoid copy std::pair<std::string, std::string> 


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:225
+  uint32_t ParentEsdId;
+  const MCSymbolGOFF *MC;
+  GOFF::ESDSymbolType SymbolType;
----------------
the name MC is easy to mess with MC layer, what about  MCSym ? 




================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:292
+  GOFFSymbol *SD;
+  const MCSectionGOFF *MC;
+  bool IsStructured;
----------------
what about to change MC to  MCSec  or other ?


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:300
+      : Pptr(nullptr), Rptr(nullptr), SD(nullptr), MC(nullptr),
+        IsStructured(false) {}
+};
----------------
we do not need the constructor,

we can change to 

```
const GOFFSymbol *Pptr=nullptr;
const GOFFSymbol *Rptr=nulllptr;
const GOFFSymbol  *SD =nullptr;
const MCSectionGOFF *MC =nullptr;
IsStructured =false;
```


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:345
+  void writeSymbol(const GOFFSymbol &Symbol, const MCAsmLayout &Layout);
+  void writeText(const GOFFSection &Section, MCAssembler &Asm,
+                 const MCAsmLayout &Layout);
----------------
change to  const MCAssembler &Asm



================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:378
+  void defineRootAndADASD(MCAssembler &Asm);
+  void defineSectionSymbols(MCAssembler &Asm, const MCSectionGOFF &Section,
+                            const MCAsmLayout &Layout);
----------------
change to const MCAssembler &Asm , there are still have some in the following code, please change them.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:390
+  GOFFSymbol *ADA = nullptr;
+  bool HasADA = false;
 };
----------------
what about move data member together?  

move above 4 data member to after
  
size_t SectionWrittenLength = 0; // Logical number of bytes written


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:435
+      createGOFFSymbol(Name, GOFF::ESD_ST_ExternalReference, ParentEsdId);
+
+  if (Source) {
----------------
if the Source must be not  nullptr, please add assert here.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:453
+  auto S = createGOFFSymbol(Name, GOFF::ESD_ST_PartReference, ParentEsdId);
+  return S;
+}
----------------
return  createGOFFSymbol(Name, GOFF::ESD_ST_PartReference, ParentEsdId);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152945