From fea4637cfe64668a8b3dc7f8d66528ad3128eadf Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 3 Sep 2018 10:15:13 +0200 Subject: [PATCH] Adds check for unused field trial parameters. This adds a dcheck to detect if a FieldTrialParameter has been created but not used in parsing a field trial. This is an easy mistake to make and cause extra work debugging why nothing happens. Also improving the ergonomics of using the parameter and optional classes. Making it easier to use them as drop in replacements for their underlying classes. In particular, the optional parameter class implements and interface more similar to the optional class. Bug: webrtc:9510 Change-Id: I5a12dd66396fa4cac9c9cf517172ae2f06984060 Reviewed-on: https://webrtc-review.googlesource.com/96761 Commit-Queue: Sebastian Jansson Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#24527} --- rtc_base/experiments/BUILD.gn | 1 + rtc_base/experiments/field_trial_parser.cc | 11 ++++++++++- rtc_base/experiments/field_trial_parser.h | 10 +++++++++- .../experiments/field_trial_parser_unittest.cc | 14 +++++++------- rtc_base/experiments/field_trial_units_unittest.cc | 8 ++++---- 5 files changed, 31 insertions(+), 13 deletions(-) diff --git a/rtc_base/experiments/BUILD.gn b/rtc_base/experiments/BUILD.gn index c0e1049b31..f5f372ff9e 100644 --- a/rtc_base/experiments/BUILD.gn +++ b/rtc_base/experiments/BUILD.gn @@ -32,6 +32,7 @@ rtc_static_library("field_trial_parser") { "../../api/units:data_rate", "../../api/units:data_size", "../../api/units:time_delta", + "../../rtc_base:checks", "../../system_wrappers:field_trial_api", "//third_party/abseil-cpp/absl/types:optional", ] diff --git a/rtc_base/experiments/field_trial_parser.cc b/rtc_base/experiments/field_trial_parser.cc index 55299adbb6..5565c8648c 100644 --- a/rtc_base/experiments/field_trial_parser.cc +++ b/rtc_base/experiments/field_trial_parser.cc @@ -14,6 +14,7 @@ #include #include +#include "rtc_base/checks.h" #include "rtc_base/logging.h" namespace webrtc { @@ -28,7 +29,10 @@ int FindOrEnd(std::string str, size_t start, char delimiter) { FieldTrialParameterInterface::FieldTrialParameterInterface(std::string key) : key_(key) {} -FieldTrialParameterInterface::~FieldTrialParameterInterface() = default; +FieldTrialParameterInterface::~FieldTrialParameterInterface() { + RTC_DCHECK(used_) << "Field trial parameter with key: '" << key_ + << "' never used."; +} std::string FieldTrialParameterInterface::Key() const { return key_; } @@ -38,6 +42,7 @@ void ParseFieldTrial( std::string trial_string) { std::map field_map; for (FieldTrialParameterInterface* field : fields) { + field->MarkAsUsed(); field_map[field->Key()] = field; } size_t i = 0; @@ -108,6 +113,10 @@ bool FieldTrialFlag::Get() const { return value_; } +webrtc::FieldTrialFlag::operator bool() const { + return value_; +} + bool FieldTrialFlag::Parse(absl::optional str_value) { // Only set the flag if there is no argument provided. if (str_value) { diff --git a/rtc_base/experiments/field_trial_parser.h b/rtc_base/experiments/field_trial_parser.h index a385ccfc16..c743bb0145 100644 --- a/rtc_base/experiments/field_trial_parser.h +++ b/rtc_base/experiments/field_trial_parser.h @@ -41,11 +41,13 @@ class FieldTrialParameterInterface { friend void ParseFieldTrial( std::initializer_list fields, std::string raw_string); + void MarkAsUsed() { used_ = true; } virtual bool Parse(absl::optional str_value) = 0; std::string Key() const; private: const std::string key_; + bool used_ = false; }; // ParseFieldTrial function parses the given string and fills the given fields @@ -68,6 +70,7 @@ class FieldTrialParameter : public FieldTrialParameterInterface { : FieldTrialParameterInterface(key), value_(default_value) {} T Get() const { return value_; } operator T() const { return Get(); } + const T* operator->() const { return &value_; } protected: bool Parse(absl::optional str_value) override { @@ -135,7 +138,11 @@ class FieldTrialOptional : public FieldTrialParameterInterface { : FieldTrialParameterInterface(key) {} FieldTrialOptional(std::string key, absl::optional default_value) : FieldTrialParameterInterface(key), value_(default_value) {} - absl::optional Get() const { return value_; } + absl::optional GetOptional() const { return value_; } + const T& Value() const { return value_.value(); } + const T& operator*() const { return value_.value(); } + const T* operator->() const { return &value_.value(); } + operator bool() const { return value_.has_value(); } protected: bool Parse(absl::optional str_value) override { @@ -162,6 +169,7 @@ class FieldTrialFlag : public FieldTrialParameterInterface { explicit FieldTrialFlag(std::string key); FieldTrialFlag(std::string key, bool default_value); bool Get() const; + operator bool() const; protected: bool Parse(absl::optional str_value) override; diff --git a/rtc_base/experiments/field_trial_parser_unittest.cc b/rtc_base/experiments/field_trial_parser_unittest.cc index 69f35bd647..954909bce4 100644 --- a/rtc_base/experiments/field_trial_parser_unittest.cc +++ b/rtc_base/experiments/field_trial_parser_unittest.cc @@ -93,20 +93,20 @@ TEST(FieldTrialParserTest, IgnoresInvalid) { TEST(FieldTrialParserTest, ParsesOptionalParameters) { FieldTrialOptional max_count("c", absl::nullopt); ParseFieldTrial({&max_count}, ""); - EXPECT_FALSE(max_count.Get().has_value()); + EXPECT_FALSE(max_count.GetOptional().has_value()); ParseFieldTrial({&max_count}, "c:10"); - EXPECT_EQ(max_count.Get().value(), 10); + EXPECT_EQ(max_count.GetOptional().value(), 10); ParseFieldTrial({&max_count}, "c"); - EXPECT_FALSE(max_count.Get().has_value()); + EXPECT_FALSE(max_count.GetOptional().has_value()); ParseFieldTrial({&max_count}, "c:20"); - EXPECT_EQ(max_count.Get().value(), 20); + EXPECT_EQ(max_count.GetOptional().value(), 20); ParseFieldTrial({&max_count}, "c:"); - EXPECT_EQ(max_count.Get().value(), 20); + EXPECT_EQ(max_count.GetOptional().value(), 20); FieldTrialOptional optional_string("s", std::string("ab")); ParseFieldTrial({&optional_string}, "s:"); - EXPECT_EQ(optional_string.Get().value(), ""); + EXPECT_EQ(optional_string.GetOptional().value(), ""); ParseFieldTrial({&optional_string}, "s"); - EXPECT_FALSE(optional_string.Get().has_value()); + EXPECT_FALSE(optional_string.GetOptional().has_value()); } TEST(FieldTrialParserTest, ParsesCustomEnumParameter) { FieldTrialEnum my_enum("e", CustomEnum::kDefault, diff --git a/rtc_base/experiments/field_trial_units_unittest.cc b/rtc_base/experiments/field_trial_units_unittest.cc index 1a2b75d646..80771d926a 100644 --- a/rtc_base/experiments/field_trial_units_unittest.cc +++ b/rtc_base/experiments/field_trial_units_unittest.cc @@ -32,19 +32,19 @@ struct DummyExperiment { TEST(FieldTrialParserUnitsTest, FallsBackToDefaults) { DummyExperiment exp(""); EXPECT_EQ(exp.target_rate.Get(), DataRate::kbps(100)); - EXPECT_FALSE(exp.max_buffer.Get().has_value()); + EXPECT_FALSE(exp.max_buffer.GetOptional().has_value()); EXPECT_EQ(exp.period.Get(), TimeDelta::ms(100)); } TEST(FieldTrialParserUnitsTest, ParsesUnitParameters) { DummyExperiment exp("t:300kbps,b:5bytes,p:300ms"); EXPECT_EQ(exp.target_rate.Get(), DataRate::kbps(300)); - EXPECT_EQ(*exp.max_buffer.Get(), DataSize::bytes(5)); + EXPECT_EQ(*exp.max_buffer.GetOptional(), DataSize::bytes(5)); EXPECT_EQ(exp.period.Get(), TimeDelta::ms(300)); } TEST(FieldTrialParserUnitsTest, ParsesDefaultUnitParameters) { DummyExperiment exp("t:300,b:5,p:300"); EXPECT_EQ(exp.target_rate.Get(), DataRate::kbps(300)); - EXPECT_EQ(*exp.max_buffer.Get(), DataSize::bytes(5)); + EXPECT_EQ(*exp.max_buffer.GetOptional(), DataSize::bytes(5)); EXPECT_EQ(exp.period.Get(), TimeDelta::ms(300)); } TEST(FieldTrialParserUnitsTest, ParsesInfinityParameter) { @@ -55,7 +55,7 @@ TEST(FieldTrialParserUnitsTest, ParsesInfinityParameter) { TEST(FieldTrialParserUnitsTest, ParsesOtherUnitParameters) { DummyExperiment exp("t:300bps,p:0.3 seconds,b:8 bytes"); EXPECT_EQ(exp.target_rate.Get(), DataRate::bps(300)); - EXPECT_EQ(*exp.max_buffer.Get(), DataSize::bytes(8)); + EXPECT_EQ(*exp.max_buffer.GetOptional(), DataSize::bytes(8)); EXPECT_EQ(exp.period.Get(), TimeDelta::ms(300)); }