[PATCH] D42222: [llvm-objcopy] Refactor llvm-objcopy to use reader and writer objects

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Okay, I like the changes now. Perhaps worth adding a test with a crazy input being converted to Binary, resulting in errors on those section types? Alternatively, instead of emitting an error, maybe it makes sense to simply write the table as some arbitrary binary blob and warn? I doubt there's a use case for this though, so I think emitting an error for now is fine, and change if the need arises.



================
Comment at: tools/llvm-objcopy/Object.cpp:73
+void BinarySectionWriter::visit(const SymbolTableSection &Sec) {
+  error("Cannot write symbol table out to bianry");
+}
----------------
bianry -> binary (also other two cases below)

I think that it would be better if these error messages had the section name included.


================
Comment at: tools/llvm-objcopy/Object.h:94-102
+class BinarySectionWriter : public SectionWriter {
+public:
+  virtual ~BinarySectionWriter() {}
+
+  void visit(const SymbolTableSection &Sec) override;
+  void visit(const RelocationSection &Sec) override;
+  void visit(const GnuDebugLinkSection &Sec) override;
----------------
To keep a consistent order of classes, this should go below ELFSectionWriter, or ELFWriter should go below BinaryWriter.


Repository:
  rL LLVM

https://reviews.llvm.org/D42222