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

Adrian Prantl via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 1 13:42:56 PDT 2019


aprantl added inline comments.


================
Comment at: include/llvm/Support/DataExtractor.h:82
   ///     NULL will be returned.
+  const char *getCStr(uint64_t *offset_ptr) const;
+
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64006





More information about the llvm-commits mailing list