[lldb/Utility] Simplify Scalar::PromoteToMaxType

[labath]

Summary

The function had very complicated signature, because it was trying to
avoid making unnecessary copies of the Scalar object. However, this
class is not hot enough to worry about these kinds of optimizations. My
making copies unconditionally, we can simplify the function and all of
its call sites.

[labath]

lldb/include/lldb/Utility/Scalar.h

268–276

If we're worried about the seemingly random usage of const&, we could also make the copies inside the functions, or change everything to use value parameters...

[JDevlieghere]

Yeah, I think this is worth the overhead of making a few copies. LGTM.

lldb/include/lldb/Utility/Scalar.h

public:
   GetValueTypeForUnsignedIntegerWithByteSize(size_t byte_size);
 
   static Scalar::Type GetValueTypeForFloatWithByteSize(size_t byte_size);
 
   // All operators can benefits from the implicit conversions that will happen
   // automagically by the compiler, so no temporary objects will need to be
   // created. As a result, we currently don't need a variety of overloaded set
   // value accessors.
-  Scalar &operator+=(const Scalar &rhs);
+  Scalar &operator+=(Scalar rhs);
   Scalar &operator<<=(const Scalar &rhs); // Shift left
   Scalar &operator>>=(const Scalar &rhs); // Shift right (arithmetic)
   Scalar &operator&=(const Scalar &rhs);
 
   // Shifts the current value to the right without maintaining the current sign
   // of the value (if it is signed).
   bool ShiftRightLogical(const Scalar &rhs); // Returns true on success
 
protected:
   // Classes that inherit from Scalar can see and modify these
   Scalar::Type m_type;
   llvm::APInt m_integer;
   llvm::APFloat m_float;
 
   template <typename T> T GetAs(T fail_value) const;
 
 private:
   friend const Scalar operator+(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator-(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator/(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator*(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator&(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator|(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator%(const Scalar &lhs, const Scalar &rhs);
-  friend const Scalar operator^(const Scalar &lhs, const Scalar &rhs);
+  friend const Scalar operator-(Scalar lhs, Scalar rhs);
+  friend const Scalar operator/(Scalar lhs, Scalar rhs);
+  friend const Scalar operator*(Scalar lhs, Scalar rhs);
+  friend const Scalar operator&(Scalar lhs, Scalar rhs);
+  friend const Scalar operator|(Scalar lhs, Scalar rhs);
+  friend const Scalar operator%(Scalar lhs, Scalar rhs);
+  friend const Scalar operator^(Scalar lhs, Scalar rhs);
   friend const Scalar operator<<(const Scalar &lhs, const Scalar &rhs);
   friend const Scalar operator>>(const Scalar &lhs, const Scalar &rhs);
-  friend bool operator==(const Scalar &lhs, const Scalar &rhs);
+  friend bool operator==(Scalar lhs, Scalar rhs);
   friend bool operator!=(const Scalar &lhs, const Scalar &rhs);
-  friend bool operator<(const Scalar &lhs, const Scalar &rhs);
+  friend bool operator<(Scalar lhs, Scalar rhs);
   friend bool operator<=(const Scalar &lhs, const Scalar &rhs);
   friend bool operator>(const Scalar &lhs, const Scalar &rhs);
   friend bool operator>=(const Scalar &lhs, const Scalar &rhs);
 };
 
 // Split out the operators into a format where the compiler will be able to
 // implicitly convert numbers into Scalar objects.
 //
 // This allows code like:
 //      Scalar two(2);
 //      Scalar four = two * 2;
 //      Scalar eight = 2 * four;    // This would cause an error if the
 //                                  // operator* was implemented as a
 //                                  // member function.
 // SEE:
 //  Item 19 of "Effective C++ Second Edition" by Scott Meyers
 //  Differentiate among members functions, non-member functions, and
 //  friend functions
 const Scalar operator+(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator-(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator/(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator*(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator&(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator|(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator%(const Scalar &lhs, const Scalar &rhs);
-const Scalar operator^(const Scalar &lhs, const Scalar &rhs);
+const Scalar operator-(Scalar lhs, Scalar rhs);
+const Scalar operator/(Scalar lhs, Scalar rhs);
+const Scalar operator*(Scalar lhs, Scalar rhs);
+const Scalar operator&(Scalar lhs, Scalar rhs);
+const Scalar operator|(Scalar lhs, Scalar rhs);
+const Scalar operator%(Scalar lhs, Scalar rhs);
+const Scalar operator^(Scalar lhs, Scalar rhs);
 const Scalar operator<<(const Scalar &lhs, const Scalar &rhs);
 const Scalar operator>>(const Scalar &lhs, const Scalar &rhs);
-bool operator==(const Scalar &lhs, const Scalar &rhs);
+bool operator==(Scalar lhs, Scalar rhs);
 bool operator!=(const Scalar &lhs, const Scalar &rhs);
-bool operator<(const Scalar &lhs, const Scalar &rhs);
+bool operator<(Scalar lhs, Scalar rhs);
 bool operator<=(const Scalar &lhs, const Scalar &rhs);
 bool operator>(const Scalar &lhs, const Scalar &rhs);
 bool operator>=(const Scalar &lhs, const Scalar &rhs);
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const Scalar &scalar);
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_SCALAR_H

lldb/source/Utility/Scalar.cpp

case Scalar::e_long_double:
     return true;
   }
   llvm_unreachable("Unhandled type!");
 }
 
 
 // Promote to max type currently follows the ANSI C rule for type promotion in
 // expressions.
-static Scalar::Type PromoteToMaxType(
-    const Scalar &lhs,  // The const left hand side object
-    const Scalar &rhs,  // The const right hand side object
-    Scalar &temp_value, // A modifiable temp value than can be used to hold
-                        // either the promoted lhs or rhs object
-    const Scalar *&promoted_lhs_ptr, // Pointer to the resulting possibly
-                                     // promoted value of lhs (at most one of
-                                     // lhs/rhs will get promoted)
-    const Scalar *&promoted_rhs_ptr  // Pointer to the resulting possibly
-                                     // promoted value of rhs (at most one of
-                                     // lhs/rhs will get promoted)
-) {
-  Scalar result;
-  // Initialize the promoted values for both the right and left hand side
-  // values to be the objects themselves. If no promotion is needed (both right
-  // and left have the same type), then the temp_value will not get used.
-  promoted_lhs_ptr = &lhs;
-  promoted_rhs_ptr = &rhs;
+static Scalar::Type PromoteToMaxType(Scalar &lhs, Scalar &rhs) {
   // Extract the types of both the right and left hand side values
   Scalar::Type lhs_type = lhs.GetType();
   Scalar::Type rhs_type = rhs.GetType();
 
-  if (lhs_type > rhs_type) {
-    // Right hand side need to be promoted
-    temp_value = rhs; // Copy right hand side into the temp value
-    if (temp_value.Promote(lhs_type)) // Promote it
-      promoted_rhs_ptr =
-          &temp_value; // Update the pointer for the promoted right hand side
-  } else if (lhs_type < rhs_type) {
-    // Left hand side need to be promoted
-    temp_value = lhs; // Copy left hand side value into the temp value
-    if (temp_value.Promote(rhs_type)) // Promote it
-      promoted_lhs_ptr =
-          &temp_value; // Update the pointer for the promoted left hand side
-  }
+  if (lhs_type > rhs_type)
+    rhs.Promote(lhs_type);
+  else if (lhs_type < rhs_type)
+    lhs.Promote(rhs_type);
 
   // Make sure our type promotion worked as expected
-  if (promoted_lhs_ptr->GetType() == promoted_rhs_ptr->GetType())
-    return promoted_lhs_ptr->GetType(); // Return the resulting max type
+  if (lhs.GetType() == rhs.GetType())
+    return lhs.GetType(); // Return the resulting max type
 
   // Return the void type (zero) if we fail to promote either of the values.
   return Scalar::e_void;
 }
 
 bool Scalar::GetData(DataExtractor &data, size_t limit_byte_size) const {
   size_t byte_size = GetByteSize();
   if (byte_size == 0) {
double Scalar::Double(double fail_value) const {
   return fail_value;
 }
 
 long double Scalar::LongDouble(long double fail_value) const {
   /// No way to get more precision at the moment.
   return static_cast<long double>(Double(fail_value));
 }
 
-Scalar &Scalar::operator+=(const Scalar &rhs) {
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((m_type = PromoteToMaxType(*this, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
+Scalar &Scalar::operator+=(Scalar rhs) {
+  Scalar copy = *this;
+  if ((m_type = PromoteToMaxType(copy, rhs)) != Scalar::e_void) {
     switch (GetCategory(m_type)) {
     case Category::Void:
       break;
     case Category::Integral:
-      m_integer = a->m_integer + b->m_integer;
+      m_integer = copy.m_integer + rhs.m_integer;
       break;
 
     case Category::Float:
-      m_float = a->m_float + b->m_float;
+      m_float = copy.m_float + rhs.m_float;
       break;
     }
   }
   return *this;
 }
 
 Scalar &Scalar::operator<<=(const Scalar &rhs) {
   if (GetCategory(m_type) == Category::Integral &&

 }
 
 const Scalar lldb_private::operator+(const Scalar &lhs, const Scalar &rhs) {
   Scalar result = lhs;
   result += rhs;
   return result;
 }
 
-const Scalar lldb_private::operator-(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator-(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void) {
     switch (GetCategory(result.m_type)) {
     case Category::Void:
       break;
     case Category::Integral:
-      result.m_integer = a->m_integer - b->m_integer;
+      result.m_integer = lhs.m_integer - rhs.m_integer;
       break;
     case Category::Float:
-      result.m_float = a->m_float - b->m_float;
+      result.m_float = lhs.m_float - rhs.m_float;
       break;
     }
   }
   return result;
 }
 
-const Scalar lldb_private::operator/(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator/(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-          Scalar::e_void &&
-      !b->IsZero()) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void &&
+      !rhs.IsZero()) {
     switch (GetCategory(result.m_type)) {
     case Category::Void:
       break;
     case Category::Integral:
       if (IsSigned(result.m_type))
-        result.m_integer = a->m_integer.sdiv(b->m_integer);
-      else 
-        result.m_integer = a->m_integer.udiv(b->m_integer);
+        result.m_integer = lhs.m_integer.sdiv(rhs.m_integer);
+      else
+        result.m_integer = lhs.m_integer.udiv(rhs.m_integer);
       return result;
     case Category::Float:
-      result.m_float = a->m_float / b->m_float;
+      result.m_float = lhs.m_float / rhs.m_float;
       return result;
     }
   }
   // For division only, the only way it should make it here is if a promotion
   // failed, or if we are trying to do a divide by zero.
   result.m_type = Scalar::e_void;
   return result;
 }
 
-const Scalar lldb_private::operator*(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator*(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void) {
     switch (GetCategory(result.m_type)) {
     case Category::Void:
       break;
     case Category::Integral:
-      result.m_integer = a->m_integer * b->m_integer;
+      result.m_integer = lhs.m_integer * rhs.m_integer;
       break;
     case Category::Float:
-      result.m_float = a->m_float * b->m_float;
+      result.m_float = lhs.m_float * rhs.m_float;
       break;
     }
   }
   return result;
 }
 
-const Scalar lldb_private::operator&(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator&(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void) {
     if (GetCategory(result.m_type) == Category::Integral)
-      result.m_integer = a->m_integer & b->m_integer;
+      result.m_integer = lhs.m_integer & rhs.m_integer;
     else
       result.m_type = Scalar::e_void;
   }
   return result;
 }
 
-const Scalar lldb_private::operator|(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator|(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void) {
     if (GetCategory(result.m_type) == Category::Integral)
-      result.m_integer = a->m_integer | b->m_integer;
+      result.m_integer = lhs.m_integer | rhs.m_integer;
     else
       result.m_type = Scalar::e_void;
   }
   return result;
 }
 
-const Scalar lldb_private::operator%(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator%(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
-    if (!b->IsZero() && GetCategory(result.m_type) == Category::Integral) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void) {
+    if (!rhs.IsZero() && GetCategory(result.m_type) == Category::Integral) {
       if (IsSigned(result.m_type))
-        result.m_integer = a->m_integer.srem(b->m_integer);
+        result.m_integer = lhs.m_integer.srem(rhs.m_integer);
       else
-        result.m_integer = a->m_integer.urem(b->m_integer);
+        result.m_integer = lhs.m_integer.urem(rhs.m_integer);
       return result;
     }
   }
   result.m_type = Scalar::e_void;
   return result;
 }
 
-const Scalar lldb_private::operator^(const Scalar &lhs, const Scalar &rhs) {
+const Scalar lldb_private::operator^(Scalar lhs, Scalar rhs) {
   Scalar result;
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
-  if ((result.m_type = PromoteToMaxType(lhs, rhs, temp_value, a, b)) !=
-      Scalar::e_void) {
+  if ((result.m_type = PromoteToMaxType(lhs, rhs)) != Scalar::e_void) {
     if (GetCategory(result.m_type) == Category::Integral)
-      result.m_integer = a->m_integer ^ b->m_integer;
+      result.m_integer = lhs.m_integer ^ rhs.m_integer;
     else
       result.m_type = Scalar::e_void;
   }
   return result;
 }
 
 const Scalar lldb_private::operator<<(const Scalar &lhs, const Scalar &rhs) {
   Scalar result = lhs;
case Scalar::e_uint512:
     m_integer = m_integer.lshr(bit_offset)
                     .zextOrTrunc(bit_size)
                     .zextOrSelf(8 * GetByteSize());
     return true;
   }
   return false;
 }
 
-bool lldb_private::operator==(const Scalar &lhs, const Scalar &rhs) {
+bool lldb_private::operator==(Scalar lhs, Scalar rhs) {
   // If either entry is void then we can just compare the types
   if (lhs.m_type == Scalar::e_void || rhs.m_type == Scalar::e_void)
     return lhs.m_type == rhs.m_type;
 
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
   llvm::APFloat::cmpResult result;
-  switch (PromoteToMaxType(lhs, rhs, temp_value, a, b)) {
+  switch (PromoteToMaxType(lhs, rhs)) {
   case Scalar::e_void:
     break;
   case Scalar::e_sint:
   case Scalar::e_uint:
   case Scalar::e_slong:
   case Scalar::e_ulong:
   case Scalar::e_slonglong:
   case Scalar::e_ulonglong:
   case Scalar::e_sint128:
   case Scalar::e_uint128:
   case Scalar::e_sint256:
   case Scalar::e_uint256:
   case Scalar::e_sint512:
   case Scalar::e_uint512:
-    return a->m_integer == b->m_integer;
+    return lhs.m_integer == rhs.m_integer;
   case Scalar::e_float:
   case Scalar::e_double:
   case Scalar::e_long_double:
-    result = a->m_float.compare(b->m_float);
+    result = lhs.m_float.compare(rhs.m_float);
     if (result == llvm::APFloat::cmpEqual)
       return true;
   }
   return false;
 }
 
 bool lldb_private::operator!=(const Scalar &lhs, const Scalar &rhs) {
   return !(lhs == rhs);
 }
 
-bool lldb_private::operator<(const Scalar &lhs, const Scalar &rhs) {
+bool lldb_private::operator<(Scalar lhs, Scalar rhs) {
   if (lhs.m_type == Scalar::e_void || rhs.m_type == Scalar::e_void)
     return false;
 
-  Scalar temp_value;
-  const Scalar *a;
-  const Scalar *b;
   llvm::APFloat::cmpResult result;
-  switch (PromoteToMaxType(lhs, rhs, temp_value, a, b)) {
+  switch (PromoteToMaxType(lhs, rhs)) {
   case Scalar::e_void:
     break;
   case Scalar::e_sint:
   case Scalar::e_slong:
   case Scalar::e_slonglong:
   case Scalar::e_sint128:
   case Scalar::e_sint256:
   case Scalar::e_sint512:
   case Scalar::e_uint512:
-    return a->m_integer.slt(b->m_integer);
+    return lhs.m_integer.slt(rhs.m_integer);
   case Scalar::e_uint:
   case Scalar::e_ulong:
   case Scalar::e_ulonglong:
   case Scalar::e_uint128:
   case Scalar::e_uint256:
-    return a->m_integer.ult(b->m_integer);
+    return lhs.m_integer.ult(rhs.m_integer);
   case Scalar::e_float:
   case Scalar::e_double:
   case Scalar::e_long_double:
-    result = a->m_float.compare(b->m_float);
+    result = lhs.m_float.compare(rhs.m_float);
     if (result == llvm::APFloat::cmpLessThan)
       return true;
   }
   return false;
 }
 
 bool lldb_private::operator<=(const Scalar &lhs, const Scalar &rhs) {
   return !(rhs < lhs);