clayborg added a comment. In D138618#4086789 <https://reviews.llvm.org/D138618#4086789>, @labath wrote:
> I'm sorry, but that patch does not fix the problem I am trying to point out. > In fact, I think it makes things a lot worse. > > We clearly have some kind of a communication problem, but I am running out of > ideas of what can I do about it. Let me try rephrasing it one more time: > > - this patch creates two path for converting a DIERef to a user_id_t -- a) > `ref.get_id()`; and b) `dwarf.GetUID(ref)` > - of those two ways, one is clearly more intuitive > - of those two ways, one is always correct > - those two ways aren't the same -- (a) is simpler; (b) is correct > - you can't fix that by simply taking (b) away. All that does is make the API > misuse even more likely. That patch essentially just deletes GetUID, and > inlines it into all its callers. > > Forget about the what the code does for a moment, and tell me which of these > snippets looks better: > i) > > if (IsValid()) > return GetDWARF()->GetUID(*this); > > ii) > > const std::optional<DIERef> &ref = this->GetDIERef(); > if (ref) > return DIERef(GetID(), ref->section(), ref->die_offset()).get_id(); > > iii) > > if (IsValid()) > return GetDIERef()->get_id(); > > Now look up the implementation and tell me which one is correct. Sorry a lot of noise on all of these things. Forget me last comment, I hit submit too quickly. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138618/new/ https://reviews.llvm.org/D138618 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
