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

[Pavel Labath via Phabricator via lldb-commits]

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