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

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo 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.

Thank you for providing an example. Yes sometimes it's hard to communicate over comments.
In this context yes first one is better.
Question is what should it look "under the hood".
For example:
DIERef::Decode
SymbolFileDWARF::GetUID
SymbolFileDWARF::DecodeUID
There are all these bit shifts scattered around.

If this is such a blocker I did expand on your diff, and added 64 bit support (it still has to be cleaned up a bit like various static constexpr probably moved out of DIERef to #define in dwarfh, but for illustrative purposes) https://reviews.llvm.org/D142779


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618