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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/DataExtractor.h:82
   ///     NULL will be returned.
+  const char *getCStr(uint64_t *offset_ptr) const;
+
----------------
aprantl wrote:
> ikudrin wrote:
> > aprantl wrote:
> > > Why do we need both variants? Would the 64-bit variant alone be sufficient?
> > In theory, yes. But the class is widely used in LLVM, and not only to parse DWARF sections. Trying to switch to `uint64_t` everywhere will result in a huge patch which will be hard to review. Hence, I decided to move gradually.
> I think I'd prefer to review one large (but mostly mechanical) patch over having two overloads that could potentially introduce subtle and hard-to-debug-bugs when, e.g., the 32-bit version is accidentally invoked where the 64-bit version was needed.
I would expect a mismatch like that would be rare - since the transition for one user would probably be fairly pervasive (since you pass around uint32_t* all over the place, so most of those will be forced to change when you change the root of that usage).

But yeah, I'd at least want this 32+64 support to be /very/ short lived through a few patches over weeks, not months/years.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64006