[PATCH] D68466: [DebugInfo] Allow <Symbol, Offset> pairs in AddressPool [NFC]

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AddressPool.cpp:85
+        MCConstantExpr::create(std::abs(Offset), Asm.OutContext);
+    const auto SymbolExpr = Entries[I.second.Number];
+    Entries[I.second.Number] =
----------------
aprantl wrote:
> This looks up the entry twice. Does this work?
> 
> ```
> auto &Entry = Entries[I.second.Number];
> SymbolExpr OldSymbolExpr = Entry;
> Entry = MCBinaryExpr::create(Op, OldSymbolExpr, OffsetExpr, Asm.OutContext);
> ```
Yes. Thanks.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AddressPool.h:21
+/// Pair of a symbol and an offset in number of bytes.
+typedef std::pair<const MCSymbol *, int> SymbolWithOffset;
+
----------------
aprantl wrote:
> I usually recommend defining a struct instead, so the members can have descriptive names for better readability. Since you still want the DenseMap to work, a trick I used elsewhere is to inherit from std::pair and provide accessors. Maybe that's excessive here.
Thanks for the suggestion! I have probably overlooked something, but when I inherited from std::pair I still had to implement DenseMapInfo for the inherited class, so all-in-all the code was about as large as implementing a normal struct, so I went with the latter (using the std::pair's `DenseMapInfo::getHashValue()` implementation though).


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

https://reviews.llvm.org/D68466