[PATCH] D61694: Boilerplate for producing XCOFF object files from the PowerPC backend.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCAsmInfoXCOFF.h:1
+//===- MCAsmInfoXCOFF.h XCOFF asm properties ------------------- *- C++ -*-===//
+//
----------------
This is missing the `-` between the filename and the description.


================
Comment at: llvm/include/llvm/MC/MCXCOFFStreamer.h:30
+
+  void reset() override { MCObjectStreamer::reset(); }
+};
----------------
I don't think overriding a function just to call the direct base version of it is necessary.


================
Comment at: llvm/include/llvm/Support/TargetRegistry.h:521
+      if (XCOFFStreamerCtorFn)
+        S = XCOFFStreamerCtorFn(T, Ctx, std::move(TAB), std::move(OW),
+                                std::move(Emitter), RelaxAll);
----------------
The use of the `Register*Streamer` functions is associated with registering the appropriate streamer factory function for the object file format given a specific architecture in `lib/Target/*/MCTargetDesc`. If there are no classes derived from `MCXCOFFStreamer`, then I don't think we need this.


================
Comment at: llvm/lib/MC/MCAsmInfoXCOFF.cpp:1
+//===- MC/MCAsmInfoXCOFF.cpp XCOFF asm properties -------------- *- C++ -*-===//
+//
----------------
Refer to the previous comment regarding a missing `-`.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:30
+                                          MCSymbolAttr Attribute) {
+  llvm_unreachable("Not implemented yet");
+  return false;
----------------
If the intent is to fail with assertions but silently continue in release, then this should be
`assert(false && "Not implemented yet");`

Please review other uses of `llvm_unreachable` in this patch.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:19
+
+class XCOFFObjectWriter : public MCObjectWriter {
+  support::endian::Writer W;
----------------
As a class defined in a `.cpp` file, this should probably go in an unnamed namespace.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:23
+
+  virtual void executePostLayoutBinding(llvm::MCAssembler &,
+                                        const llvm::MCAsmLayout &) override;
----------------
The `llvm::` here seems noisy and should be unnecessary here (and in a good number of cases in this file).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:43
+void XCOFFObjectWriter::executePostLayoutBinding(llvm::MCAssembler &,
+                                                 const llvm::MCAsmLayout &) { }
+
----------------
Does this need a TODO?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:72
+  // Number of entries in the symbol table.
+  W.write<uint32_t>(0);
+  // Size of the optional header.
----------------
`int32_t`


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:74
+  // Size of the optional header.
+  W.write<uint32_t>(0);
+  // Flags.
----------------
`uint16_t`


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:76
+  // Flags.
+  W.write<uint32_t>(0);
+
----------------
`uint16_t`


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:20
+};
+} // namespace
+
----------------
Use `// end anonymous namespace`.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:22
+
+PPCXCOFFObjectWriter::PPCXCOFFObjectWriter() : MCXCOFFObjectTargetWriter() {}
+
----------------
Use `= default`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1652
 
+bool PPCAIXAsmPrinter::doFinalization(Module &M) {
+  return AsmPrinter::doFinalization(M);
----------------
Granted, this does not override a function just to clearly invoke the implementation in the direct base class, but explicitly skipping to an indirect base class with no comment is not better. Also, the direct base class does not override `AsmPrinter::doFinalization` anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61694