Added integer parsing functions in base/string_to_number.h

webrtc/base/string_to_number_unittest.cc

+/*
+ *  Copyright 2017 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/base/string_to_number.h"
+
+#include <string>
+#include <type_traits>
+#include <limits>
+
+#include "webrtc/base/gunit.h"
+
+namespace rtc {
+
+namespace {
+// clang-format off
+typedef ::testing::Types<signed char, unsigned char,

[kwiberg-webrtc, 2017/02/16 00:27:24]

using?

[ossu, 2017/02/16 10:19:26]

Sure!

+                         short,       unsigned short,
+                         int,         unsigned int,
+                         long,        unsigned long,
+                         long long,   unsigned long long,
+                         int8_t,      uint8_t,
+                         int16_t,     uint16_t,
+                         int32_t,     uint32_t,
+                         int64_t,     uint64_t> IntegerTypes;
+
+typedef ::testing::Types<float, double, long double> FloatTypes;
+// clang-format on
+
+template <typename T>
+class BasicNumberTest : public ::testing::Test {};
+
+TYPED_TEST_CASE_P(BasicNumberTest);
+
+TYPED_TEST_P(BasicNumberTest, TestValidNumbers) {
+  const auto lowest_value = std::numeric_limits<TypeParam>::lowest();

[kwiberg-webrtc, 2017/02/16 00:27:24]

"min_value"? I don't think the lowest/min naming of numeric_limits is a good
model to emulate...

[ossu, 2017/02/16 10:19:26]

I think it would be more confusing if min meant two different things within the
same function, i.e. the definition from numeric_limits and the "regular" one. If
this were non-test code, I'd consider it more important to conform to the rest
of our code than to numeric_limits.

I'll consider it.

+  const auto max_value = std::numeric_limits<TypeParam>::max();
+  const std::string lowest_string = std::to_string(lowest_value);
+  const std::string max_string = std::to_string(max_value);
+  EXPECT_EQ(lowest_value, StringToNumber<TypeParam>(lowest_string))
+      << lowest_string << " should be parsable ";
+  EXPECT_EQ(lowest_value, StringToNumber<TypeParam>(lowest_string.c_str()))
+      << lowest_string << " should be parsable ";
+  EXPECT_EQ(max_value, StringToNumber<TypeParam>(max_string))
+      << max_string << " should be parsable ";
+  EXPECT_EQ(max_value, StringToNumber<TypeParam>(max_string.c_str()))
+      << max_string << " should be parsable ";
+  EXPECT_EQ(0, StringToNumber<TypeParam>("-0"));
+  EXPECT_EQ(0, StringToNumber<TypeParam>(std::string("-0000000000000")));

[kwiberg-webrtc, 2017/02/16 00:27:24]

Hmm. Consider "using T = TypeParam" in these methods, to reduce verbosity.

[ossu, 2017/02/16 10:19:26]

Sure!

+}

[kwiberg-webrtc, 2017/02/16 00:27:24]

Consider testing a few specific simple cases, such as 0, 1, -1, 17, etc.

+
+TYPED_TEST_P(BasicNumberTest, TestInvalidNumbers) {
+  // Value ranges aren't strictly enforced in this test, since that would either
+  // require doctoring specific strings for each data type, which is a hassle
+  // across platforms, or to be able to do addition of values larger than the
+  // largest type, which is another hassle.
+  const auto lowest_value = std::numeric_limits<TypeParam>::lowest();
+  const auto max_value = std::numeric_limits<TypeParam>::max();
+  // If the type supports negative values, make the large negative value
+  // approximately ten times larger. If the type is unsigned, just use -2.

[kwiberg-webrtc, 2017/02/16 00:27:24]

Prepending a "1" doesn't make a value approximately 10 times larger---consider
e.g. "9" -> "19". You could say "at least twice as large", or just append "0"
instead and make the comment true.

[ossu, 2017/02/16 10:19:26]

That is true. Initially I appended a "1" instead, but realized this might not
work well with floats: 1.00000011 is also not ten times larger than 1.0000001
and 1.00000010 is not larger at all. I'll have to re-think this a bit as well,
since some implementations (see failed tests) put in +/-inf for these values,
and 1inf is just garbage.

[kwiberg-webrtc, 2017/02/16 11:59:42]

Oh right, I totally forgot about the floating point case.

+  const std::string too_low_string =
+      (lowest_value == 0) ? "-2"
+                          : ("-1" + std::to_string(lowest_value).substr(1));
+  // Make the large value approximately ten times larger than the maximum.
+  const std::string too_large_string = "1" + std::to_string(max_value);
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(too_low_string))
+      << too_low_string << " should not be parsable, but parsed as "
+      << *StringToNumber<TypeParam>(too_low_string);

[kwiberg-webrtc, 2017/02/16 00:27:24]

Do you need to repeat the parsed number? Won't EXPECT_EQ print it already?

[ossu, 2017/02/16 10:19:26]

No. It can't print out things through rtc::Optional. I think we should have a
look at fixing that, since we're making more use of Optional and it's making our
tests results unreadable. Unfortunately, there doesn't seem to be an operator<<
for std::optional. gtest also supports printing an object if there's a
PrintTo(const Obj&, ostream*) function declared, so maybe we can use that?

[kwiberg-webrtc, 2017/02/16 11:59:42]

Yes, that sounds good.

+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(too_low_string.c_str()))
+      << too_low_string << " should not be parsable, but parsed as "
+      << *StringToNumber<TypeParam>(too_low_string.c_str());
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(too_large_string))
+      << too_large_string << " should not be parsable, but parsed as "
+      << *StringToNumber<TypeParam>(too_large_string);
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(too_large_string.c_str()))
+      << too_large_string << " should not be parsable, but parsed as "
+      << *StringToNumber<TypeParam>(too_large_string.c_str());
+}
+
+TYPED_TEST_P(BasicNumberTest, TestInvalidInputs) {
+  const char kInvalidCharArray[] = "Invalid string containing 47";
+  const char kPlusMinusCharArray[] = "+-100";
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(kInvalidCharArray));
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(std::string(kInvalidCharArray)));
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(kPlusMinusCharArray));
+  EXPECT_EQ(rtc::Optional<TypeParam>(),
+            StringToNumber<TypeParam>(std::string(kPlusMinusCharArray)));
+}
+
+REGISTER_TYPED_TEST_CASE_P(BasicNumberTest,
+                           TestValidNumbers,
+                           TestInvalidNumbers,
+                           TestInvalidInputs);
+
+}  // namespace
+
+INSTANTIATE_TYPED_TEST_CASE_P(StringToNumberTest_Integers,
+                              BasicNumberTest,
+                              IntegerTypes);
+
+INSTANTIATE_TYPED_TEST_CASE_P(StringToNumberTest_Floats,
+                              BasicNumberTest,
+                              FloatTypes);
+
+}  // namespace rtc