Add base::FeatureParam<> struct

base/metrics/field_trial_params.h

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_METRICS_FIELD_TRIAL_PARAMS_H_
 #define BASE_METRICS_FIELD_TRIAL_PARAMS_H_
 
 #include <map>
 #include <string>
 
@@ skipping 73 matching lines @@
 // Same as GetFieldTrialParamValueByFeature(). On top of that, it converts the
 // string value into a boolean and returns it, if successful. Otherwise, it
 // returns |default_value|. The only string representations accepted here are
 // "true" and "false". If the string value is not empty and the conversion does
 // not succeed, it produces a warning to LOG.
 BASE_EXPORT bool GetFieldTrialParamByFeatureAsBool(
     const base::Feature& feature,
     const std::string& param_name,
     bool default_value);
 
+// Shared declaration for various FeatureParam<T> types.
+//
+// TODO(sfiera): is this the way we want to parameterize for different types? I

[Alexei Svitkine (slow), 2017/04/24 18:35:47]

I think this is a good use of templates - since the type gets clearly defined at
the declaration - which is nice documentation for the param.

[sfiera, 2017/04/28 16:07:04]

Acknowledged.

+// tend to prefer template specialization when classes/functions have the same
+// functionality for different types, but separate functions are used above.
+// (Note that the T=enum case even has a slightly different API).
+template <typename T, bool IsEnum = std::is_enum<T>::value>
+struct FeatureParam {
+  // Prevent use of FeatureParam<> with unsupported types (e.g. void*). Uses T
+  // in its definition so that evaluation is deferred until the template is
+  // instantiated.
+  // TODO(sfiera): no-compile test:
+  //     https://www.chromium.org/developers/testing/no-compile-tests
+  static_assert(!std::is_same<T, T>::value, "unsupported FeatureParam<> type");
+};
+
+// Declares a string-valued parameter. Example:
+//
+//     constexpr FeatureParam<string> kAssistantName{
+//         &kAssistantFeature, "assistant_name", "HAL"};
+//
+// If the feature is not set, or set to the empty string, then Get() will return
+// the default value.
+//
+// TODO(sfiera): should Get() be a method on the object, or would it be better
+// to have a free function like GetFieldTrialParam(kAssistantName)?
+template <>
+struct FeatureParam<std::string> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         const char* default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT std::string Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const char* const default_value;
+};
+
+// Declares a double-valued parameter. Example:
+//
+//     constexpr FeatureParam<double> kAssistantTriggerThreshold{
+//         &kAssistantFeature, "trigger_threshold", 0.10};
+//
+// If the feature is not set, or set to an invalid double value, then Get() will
+// return the default value.
+template <>
+struct FeatureParam<double> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         double default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT double Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const double default_value;
+};
+
+// Declares an int-valued parameter. Example:
+//
+//     constexpr FeatureParam<int> kAssistantParallelism{
+//         &kAssistantFeature, "parallelism", 4};
+//
+// If the feature is not set, or set to an invalid double value, then Get() will
+// return the default value.
+template <>
+struct FeatureParam<int> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         int default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT int Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const int default_value;
+};
+
+template <>
+struct FeatureParam<bool> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         bool default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT bool Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const bool default_value;
+};
+
+BASE_EXPORT void LogInvalidEnumValue(const Feature& feature,
+                                     const std::string& param_name,
+                                     const std::string& value_as_string,
+                                     int default_value_as_int);
+
+// Feature param declaration for an enum, with associated options. Example:
+//
+//     constexpr FeatureParam<ShapeEnum>::Option[] kShapeParamOptions[] = {
+//         {SHAPE_CIRCLE, "circle"},
+//         {SHAPE_CYLINDER, "cylinder"},
+//         {SHAPE_PAPERCLIP, "paperclip"}};
+//     constexpr FeatureParam<ShapeEnum> kAssistantShapeParam{
+//         &kAssistantFeature, "shape", SHAPE_CIRCLE, &kShapeParamOptions};
+//
+// With this declaration, the parameter may be set to "circle", "cylinder", or
+// "paperclip", and that will be translated to one of the three enum values. By
+// default, or if the param is set to an unknown value, the parameter will be
+// assumed to be SHAPE_CIRCLE.
+//
+// TODO(sfiera): Should there be a "GetFieldTrialParamByFeatureAsEnum()"
+// function above to match the other types? Is this something we even want to
+// provide?

[Alexei Svitkine (slow), 2017/04/24 18:35:48]

I think it's not to have one given we'll be encouraging the use of these new
constructs instead.

[sfiera, 2017/04/28 16:07:04]

Acknowledged.

+template <typename Enum>
+struct FeatureParam<Enum, true> {
+  struct Option {
+    constexpr Option(Enum value, const char* name) : value(value), name(name) {}
+    Enum value;
+    const char* const name;
+  };
+
+  template <int option_count>
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         const Enum default_value,
+                         const Option (*options)[option_count])
+      : feature(feature),
+        name(name),
+        default_value(default_value),
+        options(*options),
+        option_count(option_count) {
+    // TODO(sfiera): no-compile test
+    static_assert(option_count >= 1, "FeatureParam<enum> has no options");
+  }
+
+  Enum Get() const {

[Alexei Svitkine (slow), 2017/04/24 18:35:48]

Can this be implemented out of line? If so, LogInvalidEnumValue() can be in the
anon namespace in the .cc file.

[sfiera, 2017/04/28 16:07:04]

No, unfortunately. It needs to be instantiated for each enum type.

+    std::string value = GetFieldTrialParamValueByFeature(*feature, name);
+    if (value.empty()) {

[Alexei Svitkine (slow), 2017/04/24 18:35:48]

Nit: No {}'s

[sfiera, 2017/04/28 16:07:04]

Done.

+      return default_value;
+    }
+    for (int i = 0; i < option_count; ++i) {
+      if (value == options[i].name) {
+        return options[i].value;
+      }
+    }
+    LogInvalidEnumValue(*feature, name, value, static_cast<int>(default_value));
+    return default_value;
+  }
+
+  const base::Feature* const feature;
+  const char* const name;
+  const Enum default_value;
+  const Option* const options;
+  const int option_count;
+};
+
 }  // namespace base
 
 #endif  // BASE_METRICS_FIELD_TRIAL_PARAMS_H_