Improve the functionality of JSONNumber

[tberghammer]

Summary

Improve the functionality of JSONNumber

• Add support for representing signed integers
• Add new constructors taking any signed or unsigned integer types

[clayborg]

Everything looks good, just wondering why we need the template code? See inlined comments.

include/lldb/Utility/JSON.h

102–104 ↗

(On Diff #41743)

What value is the template code giving us? Why can't we omit this and just have a constructor with int64_t and double?

[granata.enrico]

IIU my C++ correctly, this code covers neither int64_t nor double; it covers the unsigned variety of int64 - as well as any other unsigned integer type.

The main advantage (other than not relying on implicit promotions) would be that if someone writes their new integral data type (BigNum), and mark it with the appropriate traits, it would Just Work

• Enrico

[tberghammer]

include/lldb/Utility/JSON.h

102–104 ↗

(On Diff #41743)

If we have a constructor with int64_t and one with double then we will get a compilation error when we try to call it with an int32_t because the 2 overload will be ambiguous. We can work it around at each call site with casting the value to the type what the constructor takes but I would prefer to handle the issue it in 1 place instead of all call site.

[clayborg]

Ok, thanks for the explanation. You might add a comment saying something like you just said above the template stuff so people can see why it is needed.

lldb/trunk/include/lldb/Utility/JSON.h

private:
         json_string_quote_metachars (const std::string&);
         
         std::string m_data;
     };
 
     class JSONNumber : public JSONValue
     {
     public:
-        JSONNumber ();
-        explicit JSONNumber (uint64_t i);
-        explicit JSONNumber (double d);
+        typedef std::shared_ptr<JSONNumber> SP;
+
+        // We cretae a constructor for all integer and floating point type with using templates and
+        // SFINAE to avoid having ambiguous overloads because of the implicit type promotion. If we
+        // would have constructors only with int64_t, uint64_t and double types then constructing a
+        // JSONNumber from an int32_t (or any other similar type) would fail to compile.
+
+        template <typename T,
+                  typename std::enable_if<std::is_integral<T>::value &&
+                                          std::is_unsigned<T>::value>::type* = nullptr>
+        explicit JSONNumber (T u) :
+            JSONValue(JSONValue::Kind::Number),
+            m_data_type(DataType::Unsigned)
+        {
+            m_data.m_unsigned = u;
+        }
+
+        template <typename T,
+                  typename std::enable_if<std::is_integral<T>::value &&
+                                          std::is_signed<T>::value>::type* = nullptr>
+        explicit JSONNumber (T s) :
+            JSONValue(JSONValue::Kind::Number),
+            m_data_type(DataType::Signed)
+        {
+            m_data.m_signed = s;
+        }
+
+        template <typename T,
+                  typename std::enable_if<std::is_floating_point<T>::value>::type* = nullptr>
+        explicit JSONNumber (T d) :
+            JSONValue(JSONValue::Kind::Number),
+            m_data_type(DataType::Double)
+        {
+            m_data.m_double = d;
+        }
+
+        ~JSONNumber() override = default;
 
         JSONNumber (const JSONNumber& s) = delete;
         JSONNumber&
         operator = (const JSONNumber& s) = delete;
 
         void
         Write(Stream& s) override;
-        
-        typedef std::shared_ptr<JSONNumber> SP;
+        uint64_t
+        GetAsUnsigned() const;
 
         uint64_t
-        GetData () { return m_data; }
+        GetAsSigned() const;
 
         double
-        GetAsDouble()
-        {
-            if (m_is_integer)
-                return (double)m_data;
-            else
-                return m_double;
-        }
+        GetAsDouble() const;
 
         static bool classof(const JSONValue *V)
         {
             return V->GetKind() == JSONValue::Kind::Number;
         }
-        
-        ~JSONNumber() override = default;
-        
     private:
-        bool m_is_integer;
-        uint64_t m_data;
-        double m_double;
+        enum class DataType : uint8_t
+        {
+            Unsigned,
+            Signed,
+            Double
+        } m_data_type;
+
+        union
+        {
+            uint64_t m_unsigned;
+            int64_t  m_signed;
+            double   m_double;
+        } m_data;
     };
 
     class JSONTrue : public JSONValue
     {
     public:
         JSONTrue ();
 
         JSONTrue (const JSONTrue& s) = delete;

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

for ( NativeThreadProtocolSP thread_sp;
         JSONObject::SP thread_obj_sp = std::make_shared<JSONObject>();
         threads_array_sp->AppendObject(thread_obj_sp);
 
         if (JSONObject::SP registers_sp = GetRegistersAsJSON(*thread_sp, abridged))
             thread_obj_sp->SetObject("registers", registers_sp);
 
         thread_obj_sp->SetObject("tid", std::make_shared<JSONNumber>(tid));
         if (signum != 0)
-            thread_obj_sp->SetObject("signal", std::make_shared<JSONNumber>(uint64_t(signum)));
+            thread_obj_sp->SetObject("signal", std::make_shared<JSONNumber>(signum));
 
         const std::string thread_name = thread_sp->GetName ();
         if (! thread_name.empty())
             thread_obj_sp->SetObject("name", std::make_shared<JSONString>(thread_name));
 
         if (const char *stop_reason_str = GetStopReasonString(tid_stop_info.reason))
             thread_obj_sp->SetObject("reason", std::make_shared<JSONString>(stop_reason_str));
 

lldb/trunk/source/Utility/JSON.cpp

 }
 
 void
 JSONString::Write (Stream& s)
 {
     s.Printf("\"%s\"", json_string_quote_metachars(m_data).c_str());
 }
 
-JSONNumber::JSONNumber () :
-    JSONValue(JSONValue::Kind::Number),
-    m_is_integer(true),
-    m_data(0),
-    m_double(0.0)
+uint64_t
+JSONNumber::GetAsUnsigned() const
 {
+    switch (m_data_type)
+    {
+        case DataType::Unsigned:
+            return m_data.m_unsigned;
+        case DataType::Signed:
+            return (uint64_t)m_data.m_signed;
+        case DataType::Double:
+            return (uint64_t)m_data.m_double;
+    }
 }
 
-JSONNumber::JSONNumber (uint64_t i) :
-    JSONValue(JSONValue::Kind::Number),
-    m_is_integer(true),
-    m_data(i),
-    m_double(0.0)
+uint64_t
+JSONNumber::GetAsSigned() const
+{
+    switch (m_data_type)
     {
+        case DataType::Unsigned:
+            return (int64_t)m_data.m_unsigned;
+        case DataType::Signed:
+            return m_data.m_signed;
+        case DataType::Double:
+            return (int64_t)m_data.m_double;
+    }
 }
 
-
-JSONNumber::JSONNumber (double d) :
-    JSONValue(JSONValue::Kind::Number),
-    m_is_integer(false),
-    m_data(0),
-    m_double(d)
+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;
+    }
 }
 
 void
 JSONNumber::Write (Stream& s)
 {
-    if (m_is_integer)
-        s.Printf("%" PRIu64, m_data);
-    else
-        s.Printf("%g", m_double);
+    switch (m_data_type)
+    {
+        case DataType::Unsigned:
+            s.Printf("%" PRIu64, m_data.m_unsigned);
+            break;
+        case DataType::Signed:
+            s.Printf("%" PRId64, m_data.m_signed);
+            break;
+        case DataType::Double:
+            s.Printf("%g", m_data.m_double);
+            break;
+    }
 }
 
 JSONTrue::JSONTrue () :
     JSONValue(JSONValue::Kind::True)
 {
 }
 
 void
switch (token)
         case JSONParser::Token::ObjectStart:
             return ParseJSONObject();
 
         case JSONParser::Token::ArrayStart:
             return ParseJSONArray();
 
         case JSONParser::Token::Integer:
         {
+            if (value.front() == '-')
+            {
+                bool success = false;
+                int64_t sval = StringConvert::ToSInt64(value.c_str(), 0, 0, &success);
+                if (success)
+                    return JSONValue::SP(new JSONNumber(sval));
+            }
+            else
+            {
                 bool success = false;
                 uint64_t uval = StringConvert::ToUInt64(value.c_str(), 0, 0, &success);
                 if (success)
                     return JSONValue::SP(new JSONNumber(uval));
             }
+        }
             break;
 
         case JSONParser::Token::Float:
         {
             bool success = false;
             double val = StringConvert::ToDouble(value.c_str(), 0.0, &success);
             if (success)
                 return JSONValue::SP(new JSONNumber(val));