[Lldb-commits] [PATCH] D83116: [DWARFYAML] Add support for emitting multiple abbrev tables.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This seems fairly straight-forward to me, though I didn't dive into all the details.

Given the amount of changed tests, it may be nice to separate the format change from the linking patch (so in the interim state, one could generate multiple abbrev tables, but there would be no (reasonable) way to reference any table except the first one).



================
Comment at: llvm/include/llvm/ObjectYAML/DWARFEmitter.h:34
+private:
+  Data &DWARF;
+  std::map<uint64_t, uint64_t> AbbrevID2Index;
----------------
jhenderson wrote:
> Would it make any sense to merge the `DWARFYAML::Data` class and `DWARFYAML::DWARFState` class at all?
That would definitely be nice.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:89
+DWARFYAML::DWARFState::DWARFState(DWARFYAML::Data &DI, Error &Err) : DWARF(DI) {
+  ErrorAsOutParameter EAO(&Err);
+
----------------
If you'd do all this work in the factory function and then just pass in a finished map to the constructor, there'd be no need for the `ErrorAsOutParameter` thingy.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:91
+
+  for (uint64_t I = 0; I < DI.DebugAbbrev.size(); ++I) {
+    // If the abbrev table's ID isn't specified, we use the index as its ID.
----------------
consider: `for (auto &Abbr : enumerate(DI.DebugAbbrev))`


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:93-95
+    uint64_t ID = I;
+    if (DI.DebugAbbrev[I].ID)
+      ID = *DI.DebugAbbrev[I].ID;
----------------
`ID = DI.DebugAbbrev[I].ID.getValueOr(I)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83116