[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D138618#4092040 <https://reviews.llvm.org/D138618#4092040>, @clayborg wrote:

> How about we make DIERef constructor always take all the info that is needed to construct the objects correctly:
>
>   DIERef(DWARFDie die);
>   DIERef(SymbolFileDWARF *dwarf, dw_offset_t die_offset); // might not need this one?
>   DIERef(user_id_t uid);
>
> We might not need all of these. But in this case, we can't incorrectly use the APIs since all of the objects that are needed to fill it in are in the constructor args. We take away the ability to manually fill in the DWO num and other fields. Would that fix the issues you have with this patch Pavel?

Yes, I believe it would, but I do want to add two things:

- I don't consider it important whether most of the construction work happens inside the DIERef constructor, or outside of it. So, I would consider these two implementations equally fine

  DIERef(DWARFDie die); // compute this somehow
  DWARFDie::GetDIERef() { return DIERef(*this); }

vs.

  DIERef(dwo_id, type_unit_flag, die_offset, ...); // a dumb constructor
  DWARFDie::GetDIERef() { return DIERef(...); } // computation happens here

The first one is what you described -- the second one is how it roughly how it works right now.

- My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618