Issue 4128, 5138 - Add Parser and Serializer implemented in C++

compiled/String.h

Index: compiled/String.h
diff --git a/compiled/String.h b/compiled/String.h
index a031621a087d2785a68ef2be6e1ec6918b08c6bb..043fdd075df5164eed9d52caad8135d42c46938e 100644
--- a/compiled/String.h
+++ b/compiled/String.h
@@ -21,6 +21,7 @@
 #include <cstddef>
 #include <cstring>
 #include <type_traits>
+#include <utility>
 
 #include "debug.h"
 #include "library.h"
@@ -191,6 +192,38 @@ public:
       }
     }
   }
+
+  int64_t toInt64() const
+  {
+    size_type len = length();
+    if (len == 0)
+      return 0;

[Wladimir Palant, 2017/12/21 10:30:36]

What about using StringScanner so that we won't need explicit length checks and
position tracking?

+    size_type pos = 0;
+    bool negative = mBuf[0] == u'-';
+    if (negative)
+    {
+      ++pos;
+    }
+    int64_t result = 0;
+    for (; pos < len && mBuf[pos] >= u'0' && mBuf[pos] <= u'9'; ++pos)
+    {
+      result *= 10;
+      result += mBuf[pos] - u'0';
+    }

[hub, 2017/11/22 15:40:12]

No overflow checking?

int64_t is quite a lot of significant digits, but we should fail parsing if
going too far. On the 19th we should bail. (20th for uint64_t)

[hub, 2017/11/30 21:35:37]

std::numeric_limits<int64_t>::digit10 is the value you are looking for. And you
could combine toInt64() and toUInt64() with templates and use
std::numeric_limits<T>::is_signed() to reject when the value is unsigned.

I have my own version of this in https://codereview.adblockplus.org/29606600/ -
we should combine them. I don't mind actually making a separate patch that we
could review and land prior any of our reviews.

+    return negative ? -result : result;
+  }
+
+  uint64_t toUInt64() const

[Wladimir Palant, 2017/12/21 10:30:36]

Nit: This should be called toUint64().

+  {
+    uint64_t result = 0;
+    size_type len = length();
+    for (size_type pos = 0; pos < len && mBuf[pos] >= u'0' && mBuf[pos] <= u'9'; ++pos)
+    {
+      result *= 10;
+      result += mBuf[pos] - u'0';
+    }
+    return result;
+  }

[Wladimir Palant, 2017/12/21 10:30:36]

General note: this should be ok as a first iteration. However, performance of
i64 arithmetics is rather suboptimal with JavaScript using 32 bit numbers. I
filed https://issues.adblockplus.org/ticket/5732 on improving that.

 };
 
 class DependentString : public String
@@ -405,3 +438,36 @@ public:
     }
   }
 };
+
+template<typename Type>
+inline Type AdaptValue(const String& value);

[hub, 2017/11/22 15:40:12]

I would have called this "lexical_cast<>" probably in reference to the
boost::lexical_cast module AND static_cast<>. This is definitely a cast.

CastValue() would work too. Not convinced about how obvious is "AdaptValue()"

[Wladimir Palant, 2017/12/21 10:30:36]

Can't we have that as an instance method instead? String.To<Type>() probably.

+
+template<>
+inline bool AdaptValue<bool>(const String& value)
+{
+  return value == u"true"_str;
+}
+template<>
+inline int AdaptValue<int>(const String& value)
+{
+  return static_cast<int>(value.toInt64());
+}
+template<>
+inline uint64_t AdaptValue<uint64_t>(const String& value)
+{
+  return value.toUInt64();
+}
+template<>
+inline OwnedString AdaptValue<OwnedString>(const String& value)
+{
+  return OwnedString{value};
+}
+
+DependentString TrimSpaces(const String& value);
+
+inline std::pair<DependentString, DependentString> SplitString(const String& value, String::size_type separatorPos)
+{
+  const auto secondBeginPos = separatorPos < String::npos ? separatorPos + 1 : String::npos;
+  return {DependentString{value, 0, separatorPos},
+    DependentString{value, secondBeginPos, value.length() - secondBeginPos}};

[Wladimir Palant, 2017/12/21 10:30:36]

What is `value.length() - secondBeginPos` going to be if `secondBeginPos` is
`npos`? While I don't think that it will cause any trouble, please consider just
handling this case separately:

  if (separatorPos == String::npos)
    return {DependentString(value), DependentString()};

Also a nit: This should be more readable with a few additional newlines:

  return {
    DependentString{...},
    DependentString{...}
  };

This matches the spacing we are using for object literals in JavaScript.

[sergei, 2018/03/01 15:05:25]

It's handled by the constructor of DependentString.
Done.

+}

[Wladimir Palant, 2017/12/21 10:30:37]

Why aren't these helpers methods of the String class? String.trim() and
String.split() would make more sense.

[sergei, 2018/03/01 15:05:25]

These functions are rather helper functions than methods of String class, so
they are just functions.