[PATCH] D73969: [LLDB] Let DataExtractor deal with two-byte addresses

Wed Feb 5 14:36:37 PST 2020

labath added a comment.

In D73969#1860253 <https://reviews.llvm.org/D73969#1860253>, @aykevl wrote:

> > I do have one question though. Will the DataExtractor actually do something reasonable for non-power-of-2 sizes (5,6,7) ? If yes, then great -- if not, we should keep the assert as `2 || 4 || 8`.
>
> I checked, and the only places it is actually used is in `GetAddress`, `GetAddress_unchecked`, and `GetPointer`. All those end up at `GetMaxU64` (or the unchecked variant) which seems to take care of odd pointer lengths. I have added a test case to make sure this will keep working.

Awesome, thanks.

> 
> 
>> Since we are doing the same test all over `m_addr_size >= 1 && m_addr_size <= 8` can we just make it a function and avoid the repetition and potential erroneous updating later on that does not fix them all?
> 
> Right now there are asserts both when constructing/copying(?) the object (5 asserts) and at the place where `m_addr_size` is used (3 asserts). I would propose picking one place, such as where it is used. That would reduce the number of asserts to 3 and keep them close together. What do you think?

I agree only one of those two places should be enough. My idea was to make the constructors delegate to one another (if necessary by creating a private uber-constructor that everybody can delegate to). Then we could have only one assert.

But if that's hard for some reason, your proposal seems fine too.

> Sidenote: `GetAddress` and `GetPointer` are the same function but with a different name. Even the doc comment right before is almost identical. I tried tracing back where they came from and they can both be traced back to the initial LLDB checin from Apple <https://github.com/llvm/llvm-project/commit/30fdc8d841c9d24ac5f3d452b6ece84ee0ac991c>. Maybe this is worth deduplicating eventually.

Indeed, I think I'll do that when I find a bit of time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73969