[llvm-commits] CVS: llvm/include/llvm/ADT/APInt.h

[Chris Lattner]

Hi Sheng/Reid,

Some comments about the approach, on top of what Reid already sent.   
The most significant is that isSigned should go away:

> + class APInt {
> +   /// Friend Functions of APInt Declared here. For detailed  
> comments,
> +   /// see bottom of this file.
> +   friend bool isIntN(unsigned N, const APInt& APIVal);
> +   friend APInt ByteSwap(const APInt& APIVal);
> +   friend APInt LogBase2(const APInt& APIVal);
> +   friend double APIntToDouble(const APInt& APIVal);
> +   friend float APIntToFloat(const APInt& APIVal);

Instead of making these things friends, why not just add accessors  
for whatever they need?

> +   unsigned bitsnum;      ///< The number of bits.

Please name this 'NumBits' or something like that.

> +   bool isSigned;         ///< The sign flag for this APInt.

Why do you need isSigned?  This seems very strange.  It would be  
better to keep the value in 2s complement form and add an accessor to  
get the sign-bit if needed.  Having the sign bit be explicit makes  
sizeof(APInt) = 16 instead of 12.

> +   /// @returns the number of words to hold the integer value of  
> this APInt.
> +   /// Here one word's bitwidth equals to that of uint64_t.
> +   /// @brief Get the number of the words.
> +   inline unsigned numWords() const {

getNumWords()

> +     return bitsnum < 1 ? 0 : (bitsnum + APINT_BITS_PER_WORD - 1) /
> +                              APINT_BITS_PER_WORD;

You don't need the special case for bitsnum < 1.

> +   /// Create a new APInt by translating the string represented  
> integer value.
> +   APInt(std::string& Val, uint8_t radix = 10, bool sign = false);

the string should be 'const&'.  It would also be useful to have a  
version that takes a character range:


APInt(const char *StrStart, const char *StrEnd, uint8_t radix = 10);

which would allow construction of an APInt without copying the string  
data.

>
> +   /// @brief Postfix increment operator. Increments the APInt by  
> one.
> +   const APInt operator++(int);
> +   /// @brief Postfix decrement operator. Decrements the APInt by  
> one.
> +   const APInt operator--(int);

Postfix ++/-- operations are typically declared inline, as trivial  
wrappers around the prefix version.

> +   /// @brief Equality operator. Compare this APInt with the given  
> APInt& RHS
> +   /// for the validity of the equality relationship.
> +   bool operator==(const APInt& RHS) const;

== and != are often used to compare against specific values.  We  
should have specialized versions that take 'uint64_t', do you agree?   
We don't want to bloat the API and do this for every method, but I  
think equality comparisons are worthwhile.

> +   /// @brief Inequality operator. Compare this APInt with the  
> given APInt& RHS
> +   /// for the validity of the inequality relationship.
> +   bool operator!=(const APInt& RHS) const;

This should be an inline wrapper that calls !operator==.

> +   /// @returns a uint64_t value from this APInt. If this APInt  
> contains a single
> +   /// word, just returns VAL, otherwise pVal[0].
> +   inline uint64_t getValue() {
> +     if (isSingleWord())
> +       return isSigned ? ((int64_t(VAL) << (APINT_BITS_PER_WORD -  
> bitsnum)) >>
> +                          (APINT_BITS_PER_WORD - bitsnum)) :
> +                         VAL;
> +     else
> +       return pVal[0];
> +   }

This should assert if there are more than 64 bits.  Not doing so will  
make client errors very difficult to track down.

> +
> +   /// @returns the largest value for an APInt of the specified  
> bit-width and
> +   /// if isSign == true, it should be largest signed value,  
> otherwise largest
> +   /// unsigned value.
> +   /// @brief Gets max value of the APInt with bitwidth <= 64.
> +   static APInt getMaxValue(unsigned numBits, bool isSign);
> +   static APInt getMinValue(unsigned numBits, bool isSign);
>
It makes sense for these to have 'isSign' but not for APInt itself.

> +   /// @returns the all-ones value for an APInt of the specified  
> bit-width.
> +   /// @brief Get the all-ones value.
> +   static APInt getAllOnesValue(unsigned numBits);
> +
> +   /// @brief Set every bit to 1.
> +   APInt& set();
> +   /// @brief Set every bit to 0.
> +   APInt& clear();

These seems extraneous.

> +   /// @brief Toggle every bit to its opposite value.
> +   APInt& flip();

Isn't this just operator~ ?

> + /// @brief Check if the specified APInt has a N-bits integer value.
> + inline bool isIntN(unsigned N, const APInt& APIVal) {

Please eliminate this and just allow operator== to take a uint64_t  
operand.

> + /// @returns true if the argument APInt value is a sequence of ones
> + /// starting at the least significant bit with the remainder zero.
> + inline const bool isMask(unsigned numBits, const APInt& APIVal) {
> +   return APIVal && ((APIVal + 1) & APIVal) == 0;
> + }

I'm not sure what this does.

> + /// @returns the bit equivalent double.
> + /// If the APInt numBits > 64, truncated first and then convert  
> to double.
> + inline double APIntToDouble(const APInt& APIVal) {
> +   uint64_t value = APIVal.isSingleWord() ? APIVal.VAL :  
> APIVal.pVal[0];

This is not needed.  BitsToDouble should be sufficient, and it should  
only work for integers exactly 64-bits in size, which means APInt  
need not have this sort of API.  Likewise with the other convertions  
to FP values.

What you *are* missing is value conversions to FP values, i.e. "10" - 
 > "10.0".

-Chris