Add failure paths to a few JSONNumber members

[omjavaid]

Summary

This patch updates GetAsUnsigned(), GetAsSigned(), and GetAsDouble() JSONNumber functions to add failure check.

The previous code was generating compiler warnings for not being able to provide a return path for all possibilities.

[tberghammer]

These GetAs{...} functions should never fail in their current implementation as we have only 3 different data type and all of them are handled. The new function signatures you proposed are making the API significantly harder to use with no benefit. (Also they will produce clang warnings because of the default in a switch covering all cases.)

To silent the warning I would suggest to do this:

double
JSONNumber::GetAsDouble() const
{
    switch (m_data_type)
    {
        case DataType::Unsigned:
          return (double)m_data.m_unsigned;
        case DataType::Signed:
          return (double)m_data.m_signed;
        case DataType::Double:
          return m_data.m_double;
    }
    assert(false && "Unhandled data type");
    return 0;
}

In this code:

• Gcc won't emit warnings because all code path have a return value.
• Clang will emit a warning if somebody adds a new data type without handling them in these GetAs{...} functions (this is why I don't want a default case).
• We have an additional runtime check for the fail case with the assert if the warnings are ignored.

[labath]

btw, if you use llvm_unreachable("text"), then you don't need the return after that.

[omjavaid]

Correction made upstream by @tberghammer.

[tberghammer]

I don't know what correction you are referring to as I haven't made any change in JSON.{h,cpp} since you created this patch but I am happy with the current situation (no warning on clang)

[omjavaid]

Sorry @tberghammer there was corruption in my setup showing changes which i just made internally.

I have updated this diff as per suggestions.

LGTM?

[tberghammer]

LGTM

include/lldb/Utility/JSON.h

147–148

Nice catch!

include/lldb/Utility/JSON.h

public:
 
         JSONNumber (const JSONNumber& s) = delete;
         JSONNumber&
         operator = (const JSONNumber& s) = delete;
 
         void
         Write(Stream& s) override;
 
-        uint64_t
-        GetAsUnsigned() const;
+        bool
+        GetAsUnsigned(uint64_t &value) const;
 
-        uint64_t
-        GetAsSigned() const;
+        bool
+        GetAsSigned(int64_t &value) const;
 
-        double
-        GetAsDouble() const;
+        bool
+        GetAsDouble(double &value) const;
 
         static bool classof(const JSONValue *V)
         {
             return V->GetKind() == JSONValue::Kind::Number;
         }
 
     private:
         enum class DataType : uint8_t

source/Utility/JSON.cpp

 }
 
 void
 JSONString::Write (Stream& s)
 {
     s.Printf("\"%s\"", json_string_quote_metachars(m_data).c_str());
 }
 
-uint64_t
-JSONNumber::GetAsUnsigned() const
+bool
+JSONNumber::GetAsUnsigned(uint64_t &value) const
 {
     switch (m_data_type)
     {
-        case DataType::Unsigned:
-            return m_data.m_unsigned;
         case DataType::Signed:
-            return (uint64_t)m_data.m_signed;
+          value = (uint64_t)m_data.m_signed;
+          break;
         case DataType::Double:
-            return (uint64_t)m_data.m_double;
+          value = (uint64_t)m_data.m_double;
+          break;
+        case DataType::Unsigned:
+          value = m_data.m_unsigned;
+          break;
+        default:
+          return false;
     }
+    return true;
 }
 
-uint64_t
-JSONNumber::GetAsSigned() const
+bool
+JSONNumber::GetAsSigned(int64_t &value) const
 {
     switch (m_data_type)
     {
         case DataType::Unsigned:
-            return (int64_t)m_data.m_unsigned;
-        case DataType::Signed:
-            return m_data.m_signed;
+          value = (int64_t)m_data.m_unsigned;
+          break;
         case DataType::Double:
-            return (int64_t)m_data.m_double;
+          value = (int64_t)m_data.m_double;
+          break;
+        case DataType::Signed:
+          value = m_data.m_signed;
+          break;
+        default:
+          return false;
     }
+    return true;
 }
 
-double
-JSONNumber::GetAsDouble() const
+bool
+JSONNumber::GetAsDouble(double &value) const
 {
     switch (m_data_type)
     {
         case DataType::Unsigned:
-            return (double)m_data.m_unsigned;
+          value = (double)m_data.m_unsigned;
+          break;
         case DataType::Signed:
-            return (double)m_data.m_signed;
+          value = (double)m_data.m_signed;
+          break;
         case DataType::Double:
-            return m_data.m_double;
+          value = m_data.m_double;
+          break;
+        default:
+          return false;
     }
+    return true;
 }
 
 void
 JSONNumber::Write (Stream& s)
 {
     switch (m_data_type)
     {
         case DataType::Unsigned: