[PATCH] D64006: [Support] Support 64-bit offsets in DataExtractor.

Wed Jul 3 00:53:12 PDT 2019

ikudrin added a comment.

In D64006#1566831 <https://reviews.llvm.org/D64006#1566831>, @aprantl wrote:

> ...in places where it likely won't be needed any time soon, such as 64-bit DWARF parsing support.

In fact, I am going to implement 64-bit DWARF support. I started from `DataExtractor` because changing it seems inevitable for that anyway.

In D64006#1567394 <https://reviews.llvm.org/D64006#1567394>, @dblaikie wrote:

> But it sounds like @ikudrin is suggesting maybe it's not their goal to migrate from 32 to 64 entirely, and that maybe it's OK to have both APIs?

In the beginning, I also thought that having only one possible type for offsets, `uint64_t`, is preferable. But thinking of that for some time, I cannot find any strong reason not to keep both variants, apart from some kind of aesthetic sense, which is debatable. I really do not see any new harm which may result from having overloads for both 32- and 64-bit offsets. The only issue I can guess is a potential risk of wrapping a 32-bit offset value, but the current implementation is also affected by that, so nothing new is here.

Moreover, this is just a utility class. It should provide its users with functionality, not force them to satisfy its whims, especially if that complicates things for them without any noticeable value; I mean, storing 32-bit offsets and creating a temporary 64-bit variable just to call the utility class does not seem aesthetic, too. It is the callers who know how big their data is and which data type for offsets reflects their needs better.

Anyway, as I said, my main intent is to add support for 64-bit DWARF. While there are other users, `DataExtractor` is mainly used in the `DebugInfo/DWARF` library and I expect to change it to use 64-bit offsets while implementing that support. Hopefully, that will be done with a bunch of relatively small patches. After that, we can decide what to do with the remaining callers. Unfortunately, I cannot promise that this transition may be done in several weeks. It seems that two to four months is a more realistic estimation.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64006