[PATCH] D149093: [llvm-objdump] [NFC] Factor out DisassemblerTarget class.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Largely looks okay to me. Just a few small points.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1304
+public:
+  const Target *Target;
+  std::unique_ptr<const MCSubtargetInfo> SubtargetInfo;
----------------
Perhaps best to call this `TheTarget` here, to avoid unusual syntax like `const class *Target`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1313
+  DisassemblerTarget(const class Target *TheTarget, ObjectFile &Obj,
+                     std::string TripleName, std::string MCPU,
+                     SubtargetFeatures &Features);
----------------
These should be `StringRef` or `string_view` right, not passed-by-value `std::string`?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1332
+                                       SubtargetFeatures &Features) {
+  Target = TheTarget;
+  PrettyPrinter = &selectPrettyPrinter(Triple(TripleName));
----------------
This and some of the other code can move to the initialiser list, right?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1353-1356
+  ContextPtr =
+      std::make_unique<MCContext>(Triple(TripleName), AsmInfo.get(),
+                                  RegisterInfo.get(), SubtargetInfo.get());
+  Context = ContextPtr.get();
----------------
This feels a little weird to me (having two pointer members essentially to the same thing), although I see why you've done it. Did you consider using a `shared_ptr` instead? That also avoids coupling the secondary target quite so much to the primary target, so destruction order is less important.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1385
+                                       SubtargetFeatures &Features) {
+  Target = Other.Target;
+  Context = Other.Context;
----------------
This can all be done in the initialiser list, rather than the constructor body, if I'm not mistaken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149093