Noissue - implement a couple of common lexical_cast cases.

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
@@ skipping 475 matching lines @@
     String::size_type len = value.length();
     if (len == 0)
       return 0;
     String::size_type pos = 0;
     bool negative = std::numeric_limits<T>::is_signed && value[0] == u'-';
     if (negative)
     {
       ++pos;
     }
     T result = 0;
-    for (; pos < len && value[pos] >= u'0' && value[pos] <= u'9'; ++pos)

[hub, 2018/03/05 19:24:49]

If the condition `value[pos] >= u'0' && value[pos] <= u'9'` is false, then we
should have an error.

I'm comparing this to the behaviour of `boost::lexical_cast<>` where
`lexical_cast<int>(" 123")` is an error, so is `lexical_cast<int>("123 ")`,
because of the space.

This would also be consistent with `LexicalCastImpl<bool>::Convert()` above that
does a strict equality with `u"true"_str`.

[sergei, 2018/03/06 10:36:26]

Done, but I'm not sure about "123 ", anyway we can always fix it later.

-    {
-      // isDengerous is the optimization because there is no need for some checks

[hub, 2018/03/05 19:24:49]

isDangerous (typo)

[sergei, 2018/03/06 10:36:26]

Done.

+    for (; pos < len; ++pos)
+    {
+      auto c = value[pos];
+      if (c < u'0' || c > u'9')
+        return 0;
+      // isDangerous is the optimization because there is no need for some checks
       // when the values are far from edge cases.
       // It targets the normal values, when a value is prefixed with several
       // zeros additional checks start to work earlier than the actual value of
       // result reaches an edge case, but it does not affect the result.
       bool isDangerous = pos >= std::numeric_limits<T>::digits10;

[hub, 2018/03/06 14:45:53]

this check on the number of digits could also be done before the loop: if we
have more digits (`len` or `len - 1` if it has a sign) than the maximum it is an
error.

[sergei, 2018/03/06 15:41:32]

This condition is satisfied but from a different perspective and the idea of
having isDangerous here is rather for optimization, namely in order to skip
overflow checks when there is no necessity in them.

std::numeric_limits<T>::digits10 is the maximum number of digits which can be
stored in an integer type T without loss. E.g. for int8_t the range is [-128,
127], so digits10 is equal to 2, it can safely store [-99, 99], similar for
other types. So while the length of a text represented number is less than
`digits10 + 1` it's safe to multiply it by 10 and add `digit`, however when the
length reaches digits10 one should check that there is no overflow. E.g. `value
= "300"`, `result == 30`, `pos = 2`, now we need to check that there is no
overflow before multiplying it by 10, in that example there is an overflow.
Another example is when the `value = 128`, it's safe to multiply 12 by 10 but
one should not add 8, one may add only a number from [0, 7]. Just in case, it's
covered in tests, but only for (u)int32_t.

If the number exceeds the valid range, e.g. "1111" for int8_t it will fail on
the check before multiplying by 10, `127 / 10 < 111` is true. It's also covered
in the tests.

It seems not a common case when a number is too big for parsing, so IMO we
should not check it before the for-loop additionally.

BTW the implementation could also take into account '-' but I tried to not
introduce additional conditions, so there is no such check because I thought
that negative numbers are not too common in our data either. BTW, because of
similar reasons there is a support of zeros at the beginning, there is just no
additional check for them and as the result there is such feature.

[hub, 2018/03/06 15:56:45]

Acknowledged.

       // It also invalidates the parsing of too big numbers in comparison with
       // stopping when it encounters a non numerical character.
       // cast<uint8_t>(u"1230"_str) -> 0
       // cast<uint8_t>(u"123E"_str) -> 123
       if (isDangerous && std::numeric_limits<T>::max() / 10 < result)
       {
         return 0;
       }
       result *= 10;
-      uint8_t digit = value[pos] - u'0';
+      uint8_t digit = c - u'0';
       if (isDangerous && (std::numeric_limits<T>::max() - digit < result - (negative ? 1 : 0)))
       {
         return 0;
       }
       result += digit;
     }
     return negative ? -result : result;
   }
 };
 
 template<>
 inline OwnedString lexical_cast<OwnedString>(const String& value)
 {
   return OwnedString{value};
 }
 
 DependentString TrimSpaces(const String& value);
 
 // Splits the `value` string into two `DependentString`s excluding the character staying at `separatorPos`.
 // Useful for parsing.
 std::pair<DependentString, DependentString> SplitString(const String& value, String::size_type separatorPos);
 
 ABP_NS_END