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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits