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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols-mangling.test:14
+#
+# RAW_B:         00000000 <_Z4bbbbv>:
+# NICE_B:        00000000 <bbbb()>:
----------------
jhenderson wrote:
> 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 `-`.
I wasn't sure whether the use of `-` in the middle of the check prefix might conflict with the use of `-` to separate the semantic `-NOT:` and so forth at the end. But apparently that works fine.


================
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 {
----------------
jhenderson wrote:
> 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.
Yes, I see, with a lambda to extract the field of each object. I agree it's nicer to leave it as it is :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131589