[llvm-commits] [llvm] r148741 - in /llvm/trunk: include/llvm/Constants.h include/llvm/Value.h lib/VMCore/Constants.cpp lib/VMCore/LLVMContextImpl.cpp lib/VMCore/LLVMContextImpl.h

[Chris Lattner]

Hey Duncan, thanks for the review!

On Jan 30, 2012, at 9:01 AM, Duncan Sands wrote:
>> --- llvm/trunk/include/llvm/Constants.h (original)
>> +++ llvm/trunk/include/llvm/Constants.h Mon Jan 23 16:57:10 2012
>> @@ -535,6 +534,166 @@
>>      return V->getValueID() == ConstantPointerNullVal;
>>    }
>>  };
>> +
>> +//===----------------------------------------------------------------------===//
>> +/// ConstantDataSequential - A vector or array of data that contains no
>> +/// relocations, and whose element type is a simple 1/2/4/8-byte integer or
> 
> why talk about "relocations" here?  What is a "relocation"?  Don't you
> just mean that it is an array of numbers?  Same goes for the other uses
> of "relocations" later in the file.

Done.

> 
>> +public:
>> +
>> +  virtual void destroyConstant();
>> +
>> +  /// getElementAsInteger - If this is a sequential container of integers (of
>> +  /// any size), return the specified element in the low bits of a uint64_t.
>> +  uint64_t getElementAsInteger(unsigned i) const;
>> +
>> +  /// getElementAsAPFloat - If this is a sequential container of floating point
>> +  /// type, return the specified element as an APFloat.
>> +  APFloat getElementAsAPFloat(unsigned i) const;
> 
> Wouldn't it be more symmetric to also have a method "getElementAsAPInt"?

Perhaps.  In practice, this isn't actually useful to any clients, and is quite a
bit more expensive than returning a uint64_t (because APInt is non-POD).

The rest fixed, thanks again for the review!

-Chris