[PATCH] D126883: [Debuginfo][DWARF][NFC] Refactor DwarfStringPoolEntryRef.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

The code looks mechanically OK, but I'm not quite following the patch description/motivation. Perhaps it could be rephrased?

> This patch adds possibility to keep not only DwarfStringPoolEntry, but also pointer to it. The DwarfStringPoolEntryRef keeps reference to the string map entry. String map keeps string data and corresponding DwarfStringPoolEntry info.

I /think/ I'm following this part, at least. Might be worth pointing to specific members/classes - which string map in particular are you referring to here?

> Not all string map entries may be included into the result, and then not all string entries should have DwarfStringPoolEntry info.

OK, I sort of follow - this is for strings in input .o files that, due to some DWARF dead-stripping, are no longer used in the output? If that's the case, then I'd expect that the values in the map should become unique_ptrs, perhaps? (or raw pointers, if they can be allocated by a pool allocator of some kind). But I'm not sure why that would change how the *Ref class works - oh, because some users of this class don't want/need to change their map? (can you point to the different maps/use cases?)

Then maybe this class should become templated rather than using a union?



================
Comment at: llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h:23-25
+  MCSymbol *Symbol = nullptr;
+  uint64_t Offset = 0;
+  unsigned Index = 0;
----------------
What's the motivation for this change?


================
Comment at: llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h:75-76
+      return MapEntry.get<StringEntryPtr>()->first();
+    else
+      return MapEntry.get<ShortStringEntryPtr>()->first();
+  }
----------------
Remove else-after-return


================
Comment at: llvm/include/llvm/CodeGen/DwarfStringPoolEntry.h:83-84
+      return MapEntry.get<StringEntryPtr>()->second;
+    else
+      return *MapEntry.get<ShortStringEntryPtr>()->second;
   }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126883