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

compiled/String.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #pragma once
 
 #include <algorithm>
 #include <cstddef>
 #include <cstring>
 #include <type_traits>
+#include <utility>
 
 #include "debug.h"
 #include "library.h"
 
 inline void String_assert_writable(bool isWritable);
 
 class String
 {
   friend class DependentString;
   friend class OwnedString;
@@ skipping 150 matching lines @@
       // This should be more efficient with a lookup table but I couldn't measure
       // any performance difference.
       if (currChar >= u'A' && currChar <= u'Z')
         mBuf[i] = currChar + u'a' - u'A';
       else if (currChar >= 128)
       {
         mBuf[i] = CharToLower(currChar);
       }
     }
   }
+
+  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
 {
 public:
   explicit DependentString()
       : String(nullptr, 0, INVALID)
   {
   }
 
@@ skipping 194 matching lines @@
     if (negative)
       mBuf[pos++] = '-';
 
     for (int i = size - 1; i >= 0; i--)
     {
       mBuf[pos + i] = '0' + (num % 10);
       num /= 10;
     }
   }
 };
+
+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.