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

[Ayke via Phabricator via lldb-commits]

aykevl updated this revision to Diff 242694.
aykevl set the repository for this revision to rG LLVM Github Monorepo.
aykevl added a comment.

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

> 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?

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73969

Files:
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73969.242694.patch
Type: text/x-patch
Size: 4365 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200205/926a21b7/attachment.bin>