[llvm-commits] CVS: llvm/include/llvm/Constants.h Value.h

[Chris Lattner]

> For PR1043: http://llvm.org/PR1043 :
> Merge ConstantIntegral and ConstantBool into ConstantInt.
> Remove ConstantIntegral and ConstantBool from LLVM.

yay!  Some feedback below.

> +  /// @brief Static factory method for getting a ConstantInt  
> instance which
> +  /// stands for a bool value.
> +  static ConstantInt *get(bool Value) { return Value ? getTrue() :  
> getFalse();}

Please eliminate this method.  I'd rather have clients call  
ConstantInt::get(Type::Int1Ty, true), rather than overload "get".

> +  /// @returns the value of this ConstantInt only if it's a  
> boolean type.
> +  /// @brief return the boolean value of this constant.
> +  inline bool getBoolValue() const {
> +    assert(getType() == Type::BoolTy && "Should be a boolean  
> constant!");
> +    return static_cast<bool>(getZExtValue());
> +  }

This is redundant with getZExtValue(), please remove.

> +  virtual bool isAllOnesValue() const {
> +    if (getType() == Type::BoolTy) return getBoolValue() == true;
> +    return getSExtValue() == -1;
> +  }

No need for the bool special case.  Please don't mark this virtual.

>
> +  /// This function will return true iff this constant represents  
> the largest
> +  /// value that may be represented by the constant's type.
>    /// @returns true iff this is the largest value that may be  
> represented
>    /// by this type.
> +  /// @brief Determine if the value is maximal.
>    virtual bool isMaxValue(bool isSigned) const {

No need for this to be virtual any longer.

> +    if (getType() == Type::BoolTy) return getBoolValue() == true;

The special case for bool is incorrect  when isSigned = true, just  
remove it and the code is correct.

>      if (isSigned) {
>        int64_t V = getSExtValue();
>        if (V < 0) return false;    // Be careful about wrap-around  
> on 'long's
> @@ -215,11 +157,13 @@
>      return isAllOnesValue();
>    }
>
> +  /// This function will return true iff this constant represents  
> the smallest
> +  /// value that may be represented by this constant's type.
>    /// @returns true if this is the smallest value that may be  
> represented by
>    /// this type.
> -  /// @see ConstantIntegral
> -  /// @brief Override implementation
> +  /// @brief Determine if the value is minimal.
>    virtual bool isMinValue(bool isSigned) const {

No need to be virtual.

> +    if (getType() == Type::BoolTy) return getBoolValue() == false;

This special case is incorrect for bool when isSigned = true, just  
remove it.

-Chris