[llvm-commits] [llvm] r77023 - /llvm/trunk/include/llvm/Support/MemoryObject.h

[Daniel Dunbar]

Hi Sean,

Cool! A few minor comments:

On Fri, Jul 24, 2009 at 5:30 PM, Sean Callanan<scallanan at apple.com> wrote:
> +/// MemoryObject - Abstract base class for contiguous addressable memory.
> +///   Necessary for cases in which the memory is in another process, in a
> +///   file, or on a remote machine.
> +class MemoryObject {
> +public:
> +  /// Constructor     - Override as necessary.
> +  MemoryObject() {
> +  }

Since the class is abstract, we generally either make the constructor
protected or just leave it off if it is unnecessary. A virtual
destructor is needed, however.

> +
> +  /// getBase         - Returns the lowest valid address in the region.
> +  ///
> +  /// @result         - The lowest valid address.
> +  virtual uint64_t getBase() const = 0;

Should this be uintptr_t instead of uint64_t?

> +  /// getExtent       - Returns the size of the region in bytes.  (The region is
> +  ///                   contiguous, so the highest valid address of the region
> +  ///                   is getBase() + getExtent() - 1).
> +  ///
> +  /// @result         - The size of the region.
> +  virtual uint64_t getExtent() const = 0;
> +
> +  /// readByte        - Tries to read a single byte from the region.
> +  ///
> +  /// @param address  - The address of the byte, in the same space as getBase().
> +  /// @param ptr      - A pointer to a byte to be filled in.  Must be non-NULL.
> +  /// @result         - 0 if successful; -1 if not.  Failure may be due to a
> +  ///                   bounds violation or an implementation-specific error.
> +  virtual int readByte(uint64_t address, uint8_t* ptr) const = 0;

Is there a particular reason to provide this? It seems like clients
could just pass size=1 if they only want a byte. And it seems like
most implementations will want to override readBytes anyway?

> +  /// readByte        - Tries to read a contiguous range of bytes from the
> +  ///                   region, up to the end of the region.
> +  ///                   You should override this function if there is a quicker
> +  ///                   way than going back and forth with individual bytes.
> +  ///
> +  /// @param address  - The address of the first byte, in the same space as
> +  ///                   getBase().
> +  /// @param size     - The maximum number of bytes to copy.

Is it true that address + size should never overflow?

> +  /// @param buf      - A pointer to a buffer to be filled in.  Must be non-NULL
> +  ///                   and large enough to hold size bytes.
> +  /// @result         - The number of bytes copied if successful; (uint64_t)-1
> +  ///                   if not.
> +  ///                   Failure may be due to a bounds violation or an
> +  ///                   implementation-specific error.

Is there a reason to return the number of bytes copied, it is always
the size. Then it could just return true/false on success.

> +  virtual uint64_t readBytes(uint64_t address,
> +                             uint64_t size,
> +                             uint8_t* buf) const {
> +    uint64_t current = address;
> +    uint64_t limit = getBase() + getExtent();
> +    uint64_t ret = 0;
> +
> +    while(current - address < size && current < limit) {
> +      if(readByte(current, &buf[(current - address)]))
> +        return (uint64_t)-1;
> +
> +      ret++;
> +    }
> +
> +    return ret;
> +  }

This isn't quite right (current never changes). If most
implementations will override this it might be nicer to have this just
be a pure abstract base class. But if not,
--
for (uint64_t i = 0; i != size; ++i)
  if (readByte(address + i, buf + i))
    return (uint64_t)-1;
return size;
--
is simpler.

 - Daniel