[PATCH] D131589: [llvm-objdump] Handle multiple syms at same addr in disassembly.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols-mangling.test:7
+
+# RUN: yaml2obj %s -o %t.o
+
----------------
I'm wondering if this is a case where a generated-from-assembly-using-llvm-mc input might be more appropriate. Given we already need the Arm target for the disassembly, and we don't need to control any fine details of the object really, I don't think you lose any coverage, and the input file would be simpler.

It might be worth looking to do the same at some of the other tests, though I haven't tried to figure out whether the assembly equivalent would be simpler.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols-mangling.test:14
+#
+# RAW_B:         00000000 <_Z4bbbbv>:
+# NICE_B:        00000000 <bbbb()>:
----------------
FWIW, I find `_` characters in check prefixes weird. I'm also slightly hesitant, because it is easy enough to mistype an `_` as `-` (but less likely the other way around). I'm not sure you lose much by switching to `-`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1510-1511
+        // StringRefs pointing into it.
+        for (const std::string &DemangledName : DemangledSymNamesHere)
+          SymNamesHere.push_back(DemangledName);
+      } else {
----------------
simon_tatham wrote:
> jhenderson wrote:
> > I feel like this should be simplfiable to a single line, possibly using something like `SymNamesHere.insert(SymNamesHere.begin(), DemangledSymNamesHere.begin(), DemangledSymNamesHere.end());` although maybe it's unnecessarily complex. (Same goes for the loop immediately below in the `else`).
> I'm afraid I don't know enough about that kind of STL iterator idiom to see how you'd do it in the `else` loop, where you not only have to iterate over `SymbolsHere` but also extract the `Name` field of each one. You'd need some kind of lambda, or templated field extraction gadget, or something, surely?
You're looking for `std::transform` I believe in that case, though it's debatable whether it's easier to read, so what you've got is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131589