[PATCH] D149094: [llvm-objdump] [NFC] Use DisassemblerTarget for primary target in disassembleObject.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1593
+          MappingSymbols.emplace_back(Address - SectionAddr,
+                                      PrimaryIsThumb ? 's' : 'p');
         if (Name.startswith("$t"))
----------------
I'm concerned that having the Kind of a MappingSymbol stored as a char is unclear, and a little fragile, if we start diverging from the actual letter used in the mapping symbol as you are doing here in this patch. More generally, I'm not entirely convinced by the fact that here you are making decisions about the implications of the mapping symbols now, whereas before those decisions were made at the point of impact (i.e. the point at which the right target is selected). If possible, I'd prefer it if you removed this change from this patch. I'm not necessarily opposed to it going in in a different patch, but I'd want it reviewed independently and by others who are more familiar with mapping symbols.


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

https://reviews.llvm.org/D149094