[LLVMdev] On DataExtractor isValidOffset* interface

[Timur Iskhodzhanov]

Hi Ben,

Me and David were discussing ways to use DataExtractor recently and I
found one thing in the comments that seems wrong.

>From include/llvm/Support/DataExtractor.h:
336   /// Test the validity of \a offset.
337   ///
338   /// @return
339   ///     \b true if \a offset is a valid offset into the data in this
340   ///     object, \b false otherwise.
341   bool isValidOffset(uint32_t offset) const { return Data.size() > offset; }
342
343   /// Test the availability of \a length bytes of data from \a offset.
344   ///
345   /// @return
346   ///     \b true if \a offset is a valid offset and there are \a
347   ///     length bytes available at that offset, \b false otherwise.
348   bool isValidOffsetForDataOfSize(uint32_t offset, uint32_t length) const {
349     return offset + length >= offset && isValidOffset(offset + length - 1);
350   }

The "if \a offset is a valid offset" part of the comment for
isValidOffsetForDataOfSize doesn't seem right, as it doesn't actually
call isValidOffset(offset).  Specifically,
"isValidOffsetForDataOfSize(x, 0)" is equivalent to "isValidOffset(x -
1)" rather than "isValidOffset(x)", which is surprising (at least if
you don't look at the implementation).

The other thing we've discussed is how to check DE for "everything
we've read so far was at valid offsets".  The first solution that
comes to mind is "isValidOffset(NextByte - 1)"... but I wonder if
there are any typical usage patterns that should be addressed in a
more natural way.  Like "hasEverReadPastEndOfBuffer()" flag maybe?

––
Thanks,
Tim