[PATCH] D74560: Introduce DWARFDataExtractor::getInitialLength

Thu Feb 20 07:35:35 PST 2020

ikudrin marked an inline comment as done.
ikudrin added inline comments.

================
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:
> > 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.
> This behavior is stated (and painstakingly reiterated) on every DataExtractor method. It looks like **DWARF**DataExtractor does not have these comments, but then this class is much more lightly documented. Nonetheless, I think it would be good to preserve this behavior, as a matter of consistency.
Checked again and now I see it; you are right, it is indeed stated. In that case, I agree, it is better to stick with it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74560