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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
