> 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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
