[PATCH] D74560: Introduce DWARFDataExtractor::getInitialLength

Mon Feb 17 06:07:18 PST 2020

ikudrin added a comment.

The patch should probably be split into smaller, more focused ones. `DWARFDataExtractor::getInitialLength()` and its test should go first, and then distinct patches for each debug section.

================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h:45
+
+  std::pair<uint64_t, dwarf::DwarfFormat> getInitialLength(Cursor &C) const {
+    return getInitialLength(&getOffset(C), &getError(C));
----------------
labath wrote:
> ikudrin wrote:
> > Would it be simpler if you implement `getInitialLength(uint64_t *, Error *)` via `getInitialLength(Cursor &)`? You anyway create a Cursor inside `getInitialLength(uint64_t *, Error *)`.
> I don't think that would help very much (or at all). The second cursor comes from a desire to not modify the original offset in case we fail to read the entire initial length field. I am not sure if any of the potential users would really need this behavior, but that is how the rest of the DataExtractor functions operate, so I thought it'd be good to maintain that.
> 
> If we stick to that requirement, then I'd still need to have two cursors (or offsets, or something) and fiddle around with them. If we drop it, we wouldn't need the second Cursor even in the current setup.
It looks like that behavior is not stated. I hardly can remember if I saw it was used anywhere, but once I fixed an issue which was (partially) caused by it, D71704. I guess we can easily drop it if that helps to simplify the code, maybe adding a warning note in the comments of the methods.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74560