labath added a comment.
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.
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
