Higuoxing added a comment.




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

>>! @labath :
> That would definitely be nice.

I think they can be merged. But is there any particular reason to merge them? 
Personally, I would like to keep the `DWARFYAML::DWARFState` class. Here are a 
few concerns I would like to address:

- If we merge the `DWARFYAML::DWARFState` and `DWARFYAML::Data` at all, we will 
need to add some helper variables, e.g. `AbbrevTableID2Index` and a method like 
`Error DWARFYAML::link()` to `DWARFYAML::Data` class to link DWARF sections. We 
will have to call this special method before DWARF sections being emitted. If 
we port DWARF support to other binary format in the future, e.g., wasm, we 
might forget to call this method. If we make DWARF emitters accept 
`DWARFYAML::DWARFState`, it's easy to figure out that we need a `DWARFState` 
before emitting DWARF sections.

- Besides, I think `DWARFYAML::Data` and `DWARFYAML::DWARFState` have different 
functions. `DWARFYAML::Data` is used to contain original DWARF section 
descriptions and `DWARFYAML::DWARFState` contains helper structs and methods 
which helps link DWARF sections. It might be good not to merge them so that we 
can easily figure out when and where are those sections get linked?

I can be persuaded :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83116

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to