From e74156f7d05cf3c9858e554789b3f4bb3b93cc19 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Wed, 4 Sep 2019 12:54:42 +0200 Subject: [PATCH] Removes string support in field trial parser. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prepares for simplifying the behavior of optionals so that an empty parameter value resets the optional. Bug: webrtc:9883 Change-Id: I8ef8fe9698235044cac66bc4a587abe874c8f854 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150883 Commit-Queue: Sebastian Jansson Reviewed-by: Björn Terelius Cr-Commit-Position: refs/heads/master@{#29061} --- .../experiments/field_trial_list_unittest.cc | 60 ++++++++----------- rtc_base/experiments/field_trial_parser.cc | 7 --- rtc_base/experiments/field_trial_parser.h | 7 +-- .../field_trial_parser_unittest.cc | 22 ++----- 4 files changed, 31 insertions(+), 65 deletions(-) diff --git a/rtc_base/experiments/field_trial_list_unittest.cc b/rtc_base/experiments/field_trial_list_unittest.cc index a1abfe4bf8..99066cc144 100644 --- a/rtc_base/experiments/field_trial_list_unittest.cc +++ b/rtc_base/experiments/field_trial_list_unittest.cc @@ -20,14 +20,14 @@ namespace webrtc { struct Garment { int price = 0; - std::string color = ""; + TimeDelta age = TimeDelta::Zero(); // Only needed for testing. Garment() = default; - Garment(int p, std::string c) : price(p), color(c) {} + Garment(int p, TimeDelta a) : price(p), age(a) {} bool operator==(const Garment& other) const { - return price == other.price && color == other.color; + return price == other.price && age == other.age; } }; @@ -43,34 +43,23 @@ TEST(FieldTrialListTest, ParsesListParameter) { EXPECT_THAT(my_list.Get(), ElementsAre(1, 2, 3)); ParseFieldTrial({&my_list}, "l:-1"); EXPECT_THAT(my_list.Get(), ElementsAre(-1)); - - FieldTrialList another_list("l", {"hat"}); - EXPECT_THAT(another_list.Get(), ElementsAre("hat")); - ParseFieldTrial({&another_list}, "l"); - EXPECT_THAT(another_list.Get(), IsEmpty()); - ParseFieldTrial({&another_list}, "l:"); - EXPECT_THAT(another_list.Get(), ElementsAre("")); - ParseFieldTrial({&another_list}, "l:scarf|hat|mittens"); - EXPECT_THAT(another_list.Get(), ElementsAre("scarf", "hat", "mittens")); - ParseFieldTrial({&another_list}, "l:scarf"); - EXPECT_THAT(another_list.Get(), ElementsAre("scarf")); } // Normal usage. TEST(FieldTrialListTest, ParsesStructList) { FieldTrialStructList my_list( - {FieldTrialStructMember("color", [](Garment* g) { return &g->color; }), + {FieldTrialStructMember("age", [](Garment* g) { return &g->age; }), FieldTrialStructMember("price", [](Garment* g) { return &g->price; })}, - {{1, "blue"}, {2, "red"}}); + {{1, TimeDelta::seconds(100)}, {2, TimeDelta::PlusInfinity()}}); ParseFieldTrial({&my_list}, - "color:mauve|red|gold," + "age:inf|10s|80ms," "price:10|20|30," "other_param:asdf"); - ASSERT_THAT(my_list.Get(), - ElementsAre(Garment{10, "mauve"}, Garment{20, "red"}, - Garment{30, "gold"})); + ASSERT_THAT(my_list.Get(), ElementsAre(Garment{10, TimeDelta::PlusInfinity()}, + Garment{20, TimeDelta::seconds(10)}, + Garment{30, TimeDelta::ms(80)})); } // One FieldTrialList has the wrong length, so we use the user-provided default @@ -78,54 +67,57 @@ TEST(FieldTrialListTest, ParsesStructList) { TEST(FieldTrialListTest, StructListKeepsDefaultWithMismatchingLength) { FieldTrialStructList my_list( {FieldTrialStructMember("wrong_length", - [](Garment* g) { return &g->color; }), + [](Garment* g) { return &g->age; }), FieldTrialStructMember("price", [](Garment* g) { return &g->price; })}, - {{1, "blue"}, {2, "red"}}); + {{1, TimeDelta::seconds(100)}, {2, TimeDelta::PlusInfinity()}}); ParseFieldTrial({&my_list}, - "wrong_length:mauve|magenta|chartreuse|indigo," + "wrong_length:3|2|4|3," "garment:hat|hat|crown," "price:10|20|30"); ASSERT_THAT(my_list.Get(), - ElementsAre(Garment{1, "blue"}, Garment{2, "red"})); + ElementsAre(Garment{1, TimeDelta::seconds(100)}, + Garment{2, TimeDelta::PlusInfinity()})); } // One list is missing. We set the values we're given, and the others remain // as whatever the Garment default constructor set them to. TEST(FieldTrialListTest, StructListUsesDefaultForMissingList) { FieldTrialStructList my_list( - {FieldTrialStructMember("color", [](Garment* g) { return &g->color; }), + {FieldTrialStructMember("age", [](Garment* g) { return &g->age; }), FieldTrialStructMember("price", [](Garment* g) { return &g->price; })}, - {{1, "blue"}, {2, "red"}}); + {{1, TimeDelta::seconds(100)}, {2, TimeDelta::PlusInfinity()}}); ParseFieldTrial({&my_list}, "price:10|20|30"); - ASSERT_THAT(my_list.Get(), - ElementsAre(Garment{10, ""}, Garment{20, ""}, Garment{30, ""})); + ASSERT_THAT(my_list.Get(), ElementsAre(Garment{10, TimeDelta::Zero()}, + Garment{20, TimeDelta::Zero()}, + Garment{30, TimeDelta::Zero()})); } // The user haven't provided values for any lists, so we use the default list. TEST(FieldTrialListTest, StructListUsesDefaultListWithoutValues) { FieldTrialStructList my_list( - {FieldTrialStructMember("color", [](Garment* g) { return &g->color; }), + {FieldTrialStructMember("age", [](Garment* g) { return &g->age; }), FieldTrialStructMember("price", [](Garment* g) { return &g->price; })}, - {{1, "blue"}, {2, "red"}}); + {{1, TimeDelta::seconds(100)}, {2, TimeDelta::PlusInfinity()}}); ParseFieldTrial({&my_list}, ""); ASSERT_THAT(my_list.Get(), - ElementsAre(Garment{1, "blue"}, Garment{2, "red"})); + ElementsAre(Garment{1, TimeDelta::seconds(100)}, + Garment{2, TimeDelta::PlusInfinity()})); } // Some lists are provided and all are empty, so we return a empty list. TEST(FieldTrialListTest, StructListHandlesEmptyLists) { FieldTrialStructList my_list( - {FieldTrialStructMember("color", [](Garment* g) { return &g->color; }), + {FieldTrialStructMember("age", [](Garment* g) { return &g->age; }), FieldTrialStructMember("price", [](Garment* g) { return &g->price; })}, - {{1, "blue"}, {2, "red"}}); + {{1, TimeDelta::seconds(100)}, {2, TimeDelta::PlusInfinity()}}); - ParseFieldTrial({&my_list}, "color,price"); + ParseFieldTrial({&my_list}, "age,price"); ASSERT_EQ(my_list.Get().size(), 0u); } diff --git a/rtc_base/experiments/field_trial_parser.cc b/rtc_base/experiments/field_trial_parser.cc index 5f33b6eff8..1442c0106b 100644 --- a/rtc_base/experiments/field_trial_parser.cc +++ b/rtc_base/experiments/field_trial_parser.cc @@ -139,11 +139,6 @@ absl::optional ParseTypedParameter(std::string str) { return absl::nullopt; } -template <> -absl::optional ParseTypedParameter(std::string str) { - return std::move(str); -} - template <> absl::optional> ParseTypedParameter>( std::string str) { @@ -226,7 +221,6 @@ template class FieldTrialParameter; template class FieldTrialParameter; template class FieldTrialParameter; template class FieldTrialParameter; -template class FieldTrialParameter; template class FieldTrialConstrained; template class FieldTrialConstrained; @@ -236,6 +230,5 @@ template class FieldTrialOptional; template class FieldTrialOptional; template class FieldTrialOptional; template class FieldTrialOptional; -template class FieldTrialOptional; } // namespace webrtc diff --git a/rtc_base/experiments/field_trial_parser.h b/rtc_base/experiments/field_trial_parser.h index 42535ed6a4..9ab2900811 100644 --- a/rtc_base/experiments/field_trial_parser.h +++ b/rtc_base/experiments/field_trial_parser.h @@ -244,11 +244,9 @@ template <> absl::optional ParseTypedParameter(std::string str); template <> absl::optional ParseTypedParameter(std::string str); + template <> absl::optional ParseTypedParameter(std::string str); -template <> -absl::optional ParseTypedParameter(std::string str); - template <> absl::optional> ParseTypedParameter>( std::string str); @@ -270,8 +268,6 @@ extern template class FieldTrialParameter; extern template class FieldTrialParameter; // Interpreted using sscanf %u. extern template class FieldTrialParameter; -// Using the given value as is. -extern template class FieldTrialParameter; extern template class FieldTrialConstrained; extern template class FieldTrialConstrained; @@ -281,7 +277,6 @@ extern template class FieldTrialOptional; extern template class FieldTrialOptional; extern template class FieldTrialOptional; extern template class FieldTrialOptional; -extern template class FieldTrialOptional; } // namespace webrtc diff --git a/rtc_base/experiments/field_trial_parser_unittest.cc b/rtc_base/experiments/field_trial_parser_unittest.cc index d36b3c7d95..92649b4bf1 100644 --- a/rtc_base/experiments/field_trial_parser_unittest.cc +++ b/rtc_base/experiments/field_trial_parser_unittest.cc @@ -25,17 +25,13 @@ struct DummyExperiment { FieldTrialParameter retries = FieldTrialParameter("r", 5); FieldTrialParameter size = FieldTrialParameter("s", 3); FieldTrialParameter ping = FieldTrialParameter("p", 0); - FieldTrialParameter hash = - FieldTrialParameter("h", "a80"); explicit DummyExperiment(std::string field_trial) { - ParseFieldTrial({&enabled, &factor, &retries, &size, &ping, &hash}, - field_trial); + ParseFieldTrial({&enabled, &factor, &retries, &size, &ping}, field_trial); } DummyExperiment() { std::string trial_string = field_trial::FindFullName(kDummyExperiment); - ParseFieldTrial({&enabled, &factor, &retries, &size, &ping, &hash}, - trial_string); + ParseFieldTrial({&enabled, &factor, &retries, &size, &ping}, trial_string); } }; @@ -48,18 +44,17 @@ enum class CustomEnum { } // namespace TEST(FieldTrialParserTest, ParsesValidParameters) { - DummyExperiment exp("Enabled,f:-1.7,r:2,s:10,p:1,h:x7c"); + DummyExperiment exp("Enabled,f:-1.7,r:2,s:10,p:1"); EXPECT_TRUE(exp.enabled.Get()); EXPECT_EQ(exp.factor.Get(), -1.7); EXPECT_EQ(exp.retries.Get(), 2); EXPECT_EQ(exp.size.Get(), 10u); EXPECT_EQ(exp.ping.Get(), true); - EXPECT_EQ(exp.hash.Get(), "x7c"); } TEST(FieldTrialParserTest, InitializesFromFieldTrial) { test::ScopedFieldTrials field_trials( "WebRTC-OtherExperiment/Disabled/" - "WebRTC-DummyExperiment/Enabled,f:-1.7,r:2,s:10,p:1,h:x7c/" + "WebRTC-DummyExperiment/Enabled,f:-1.7,r:2,s:10,p:1/" "WebRTC-AnotherExperiment/Enabled,f:-3.1,otherstuff:beef/"); DummyExperiment exp; EXPECT_TRUE(exp.enabled.Get()); @@ -67,7 +62,6 @@ TEST(FieldTrialParserTest, InitializesFromFieldTrial) { EXPECT_EQ(exp.retries.Get(), 2); EXPECT_EQ(exp.size.Get(), 10u); EXPECT_EQ(exp.ping.Get(), true); - EXPECT_EQ(exp.hash.Get(), "x7c"); } TEST(FieldTrialParserTest, UsesDefaults) { DummyExperiment exp(""); @@ -76,7 +70,6 @@ TEST(FieldTrialParserTest, UsesDefaults) { EXPECT_EQ(exp.retries.Get(), 5); EXPECT_EQ(exp.size.Get(), 3u); EXPECT_EQ(exp.ping.Get(), false); - EXPECT_EQ(exp.hash.Get(), "a80"); } TEST(FieldTrialParserTest, CanHandleMixedInput) { DummyExperiment exp("p:true,h:,Enabled"); @@ -85,7 +78,6 @@ TEST(FieldTrialParserTest, CanHandleMixedInput) { EXPECT_EQ(exp.retries.Get(), 5); EXPECT_EQ(exp.size.Get(), 3u); EXPECT_EQ(exp.ping.Get(), true); - EXPECT_EQ(exp.hash.Get(), ""); } TEST(FieldTrialParserTest, ParsesDoubleParameter) { FieldTrialParameter double_param("f", 0.0); @@ -109,7 +101,6 @@ TEST(FieldTrialParserTest, IgnoresInvalid) { EXPECT_EQ(exp.retries.Get(), 5); EXPECT_EQ(exp.size.Get(), 3u); EXPECT_EQ(exp.ping.Get(), false); - EXPECT_EQ(exp.hash.Get(), "a80"); } TEST(FieldTrialParserTest, IgnoresOutOfRange) { FieldTrialConstrained low("low", 10, absl::nullopt, 100); @@ -159,11 +150,6 @@ TEST(FieldTrialParserTest, ParsesOptionalParameters) { ParseFieldTrial({&max_size}, "c:20"); EXPECT_EQ(max_size.GetOptional().value(), 20u); - FieldTrialOptional optional_string("s", std::string("ab")); - ParseFieldTrial({&optional_string}, "s:"); - EXPECT_EQ(optional_string.GetOptional().value(), ""); - ParseFieldTrial({&optional_string}, "s"); - EXPECT_FALSE(optional_string.GetOptional().has_value()); } TEST(FieldTrialParserTest, ParsesCustomEnumParameter) { FieldTrialEnum my_enum("e", CustomEnum::kDefault,