[LLVMdev] On DataExtractor isValidOffset* interface

[Jeremy Lakeman]

While equivalent, would it be easier to understand if the two functions
were flipped around?

bool isValidOffset(uint32_t offset) const { return
isValidOffsetForDataOfSize(offset,
1); }
bool isValidOffsetForDataOfSize(uint32_t offset, uint32_t length) const {
return offset + length >= offset && Data.size() >= offset+length; }


On Fri, Oct 24, 2014 at 8:13 PM, Benjamin Kramer <benny.kra at gmail.com>
wrote:

>
> > On 23.10.2014, at 21:52, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> >
> > 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).
>
> length=0 is an edge case for this function, it's designed to validate a
> read of a non-zero size from a specific offset. If you have a use case you
> can just make it call isValidOffset(Offset) in that case. This also
> explains the 'surprising' -1 behavior as for a read of 1 byte you only have
> to check if the offset is valid.
>
> DataExtractor was initially designed after a LLDB class of the same name,
> this function seems to have diverged a lot since :(
>
> > 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?
>
> Adding flags to DataExtractor wouldn't be nice as it's designed to have as
> little mutable state as possible, better add your own class on top of DE if
> you need that behavior.
>
> - Ben
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141029/f2a6c2d8/attachment.html>