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 <srte@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24527}

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",
   ]

rtc_base/experiments/field_trial_parser.cc

@@ -14,6 +14,7 @@
 #include <type_traits>
 #include <utility>
 
+#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<std::string, FieldTrialParameterInterface*> 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<std::string> str_value) {
   // Only set the flag if there is no argument provided.
   if (str_value) {

rtc_base/experiments/field_trial_parser.h

@@ -41,11 +41,13 @@ class FieldTrialParameterInterface {
   friend void ParseFieldTrial(
       std::initializer_list<FieldTrialParameterInterface*> fields,
       std::string raw_string);
+  void MarkAsUsed() { used_ = true; }
   virtual bool Parse(absl::optional<std::string> 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<std::string> str_value) override {
@@ -135,7 +138,11 @@ class FieldTrialOptional : public FieldTrialParameterInterface {
       : FieldTrialParameterInterface(key) {}
   FieldTrialOptional(std::string key, absl::optional<T> default_value)
       : FieldTrialParameterInterface(key), value_(default_value) {}
-  absl::optional<T> Get() const { return value_; }
+  absl::optional<T> 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<std::string> 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<std::string> str_value) override;

rtc_base/experiments/field_trial_parser_unittest.cc

@@ -93,20 +93,20 @@ TEST(FieldTrialParserTest, IgnoresInvalid) {
 TEST(FieldTrialParserTest, ParsesOptionalParameters) {
   FieldTrialOptional<int> 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<std::string> 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<CustomEnum> my_enum("e", CustomEnum::kDefault,

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));
 }