[PATCH] D14674: [RuntimeDyld] Add accessors to `SectionEntry`; NFC

[Andy Kaylor via llvm-commits]

andrew.w.kaylor added a comment.

In http://reviews.llvm.org/D14674#294356, @sanjoy wrote:

> In http://reviews.llvm.org/D14674#294341, @andrew.w.kaylor wrote:
>
> > It seems like "Section.getAddress() + Offset" shows up pretty frequently.  Do you think it's worth making a function to do that too?
> >
> > Otherwise, this all looks pretty straightforward.  All of my comments are about cleaning up things that exist in the current code.
>
>
> Thanks for the review!
>
> About `getAddress` -- what do you think of an interface like
>
>   template<typename T = uint8_t>
>   T* getAddress(size_t Offset = 0) {
>     return static_cast<T *>(Address + Offset);
>   }
>
>
> ?  That'll let us get rid of of the casts, and also add some bounds checking to `getAddress` in http://reviews.llvm.org/D14675.


I'd prefer to see getAddressWithOffset(size_t Offset) as a separate function, but I do think it would be great to add bounds checking for this.

As for the template idea, you'd have to use reinterpret_cast to make the conversions used work, and I'd rather see the reinterpret_cast at the point of use so that it is obvious that such a heavy-handed cast is being used.  Also, I think using a template in the offset case might make it unclear what's happening in a case like this (from the current code):

  uint32_t *TargetPtr = reinterpret_cast<uint32_t *>(Section.getAddress() + Offset);

because this

  uint32_t *TargetPtr = Section.getAddressWithOffset<uint32_t *>(Offset);

makes it ambigous as to whether the address is offset by Offset bytes or Offset uint32_t's.

I think this is easier to comprehend this way:

  uint32_t *TargetPtr = reinterpret_cast<uint32_t*>(Section.getAddressWithOffset(Offset));

Even like that the function declaration will require a clear comment about what the offset parameter means.


http://reviews.llvm.org/D14674