Export of internal Abseil changes

--
dea3e4f33f16bdb1d89cad1f8055b81c0c0cb554 by Andy Getzendanner <durandal@google.com>:

Validate in log_severity_test that flags of type absl::LogSeverity are lock-free.

PiperOrigin-RevId: 293454285

--
2a0cd2d8dc193a0cbff4ffa6c5c7037745507419 by Derek Mauro <dmauro@google.com>:

Update the testing instructions in CONTRIBUTING.md

PiperOrigin-RevId: 293436013

--
cec91c3f635b0b4c8a60955e5926dba4ed980898 by Gennadiy Rozental <rogeeff@google.com>:

Introduce struct to represent storage for flag value and normalize naming of internal structs in Flag implementation.

There is no semantic changes in this CL. All the internal structs are now named as Flag... We also stop using flags_internal:: qualifications for most of them since the names are unique enough by themselves.

PiperOrigin-RevId: 293251467
GitOrigin-RevId: dea3e4f33f16bdb1d89cad1f8055b81c0c0cb554
Change-Id: I161aecc9509edae3e4b77eead02df684b2ce7087

1 files changed, 199 insertions, 173 deletions

diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h
index 4341f1137d9d..cc07dce1829b 100644
--- a/absl/flags/internal/flag.h
+++ b/absl/flags/internal/flag.h
@@ -22,6 +22,7 @@
 #include <cstring>
 #include <memory>
 #include <string>
+#include <type_traits>
 
 #include "absl/base/config.h"
 #include "absl/base/thread_annotations.h"
@@ -37,65 +38,12 @@ namespace absl {
 ABSL_NAMESPACE_BEGIN
 namespace flags_internal {
 
-// The minimum atomic size we believe to generate lock free code, i.e. all
-// trivially copyable types not bigger this size generate lock free code.
-static constexpr int kMinLockFreeAtomicSize = 8;
-
-// The same as kMinLockFreeAtomicSize but maximum atomic size. As double words
-// might use two registers, we want to dispatch the logic for them.
-#if defined(ABSL_FLAGS_INTERNAL_ATOMIC_DOUBLE_WORD)
-static constexpr int kMaxLockFreeAtomicSize = 16;
-#else
-static constexpr int kMaxLockFreeAtomicSize = 8;
-#endif
-
-// We can use atomic in cases when it fits in the register, trivially copyable
-// in order to make memcpy operations.
-template <typename T>
-struct IsAtomicFlagTypeTrait {
-  static constexpr bool value =
-      (sizeof(T) <= kMaxLockFreeAtomicSize &&
-       type_traits_internal::is_trivially_copyable<T>::value);
-};
-
-// Clang does not always produce cmpxchg16b instruction when alignment of a 16
-// bytes type is not 16.
-struct alignas(16) FlagsInternalTwoWordsType {
-  int64_t first;
-  int64_t second;
-};
-
-constexpr bool operator==(const FlagsInternalTwoWordsType& that,
-                          const FlagsInternalTwoWordsType& other) {
-  return that.first == other.first && that.second == other.second;
-}
-constexpr bool operator!=(const FlagsInternalTwoWordsType& that,
-                          const FlagsInternalTwoWordsType& other) {
-  return !(that == other);
-}
-
-constexpr int64_t SmallAtomicInit() { return 0xababababababababll; }
-
-template <typename T, typename S = void>
-struct BestAtomicType {
-  using type = int64_t;
-  static constexpr int64_t AtomicInit() { return SmallAtomicInit(); }
-};
-
-template <typename T>
-struct BestAtomicType<
-    T, typename std::enable_if<(kMinLockFreeAtomicSize < sizeof(T) &&
-                                sizeof(T) <= kMaxLockFreeAtomicSize),
-                               void>::type> {
-  using type = FlagsInternalTwoWordsType;
-  static constexpr FlagsInternalTwoWordsType AtomicInit() {
-    return {SmallAtomicInit(), SmallAtomicInit()};
-  }
-};
-
 template <typename T>
 class Flag;
 
+///////////////////////////////////////////////////////////////////////////////
+// Persistent state of the flag data.
+
 template <typename T>
 class FlagState : public flags_internal::FlagStateInterface {
  public:
@@ -123,24 +71,27 @@ class FlagState : public flags_internal::FlagStateInterface {
   int64_t counter_;
 };
 
+///////////////////////////////////////////////////////////////////////////////
+// Flag help auxiliary structs.
+
 // This is help argument for absl::Flag encapsulating the string literal pointer
 // or pointer to function generating it as well as enum descriminating two
 // cases.
 using HelpGenFunc = std::string (*)();
 
-union FlagHelpSrc {
-  constexpr explicit FlagHelpSrc(const char* help_msg) : literal(help_msg) {}
-  constexpr explicit FlagHelpSrc(HelpGenFunc help_gen) : gen_func(help_gen) {}
+union FlagHelpMsg {
+  constexpr explicit FlagHelpMsg(const char* help_msg) : literal(help_msg) {}
+  constexpr explicit FlagHelpMsg(HelpGenFunc help_gen) : gen_func(help_gen) {}
 
   const char* literal;
   HelpGenFunc gen_func;
 };
 
-enum class FlagHelpSrcKind : int8_t { kLiteral, kGenFunc };
+enum class FlagHelpKind : int8_t { kLiteral, kGenFunc };
 
-struct HelpInitArg {
-  FlagHelpSrc source;
-  FlagHelpSrcKind kind;
+struct FlagHelpArg {
+  FlagHelpMsg source;
+  FlagHelpKind kind;
 };
 
 extern const char kStrippedFlagHelp[];
@@ -172,17 +123,18 @@ constexpr const char* HelpConstexprWrap(char* p) { return p; }
 // evaluatable in constexpr context, but the cost is an extra function being
 // generated in the ABSL_FLAG code.
 template <typename T, int = (T::Const(), 1)>
-constexpr flags_internal::HelpInitArg HelpArg(int) {
-  return {flags_internal::FlagHelpSrc(T::Const()),
-          flags_internal::FlagHelpSrcKind::kLiteral};
+constexpr FlagHelpArg HelpArg(int) {
+  return {FlagHelpMsg(T::Const()), FlagHelpKind::kLiteral};
 }
 
 template <typename T>
-constexpr flags_internal::HelpInitArg HelpArg(char) {
-  return {flags_internal::FlagHelpSrc(&T::NonConst),
-          flags_internal::FlagHelpSrcKind::kGenFunc};
+constexpr FlagHelpArg HelpArg(char) {
+  return {FlagHelpMsg(&T::NonConst), FlagHelpKind::kGenFunc};
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Flag default value auxiliary structs.
+
 // Signature for the function generating the initial flag value (usually
 // based on default value supplied in flag's definition)
 using FlagDfltGenFunc = void* (*)();
@@ -197,32 +149,138 @@ union FlagDefaultSrc {
 
 enum class FlagDefaultSrcKind : int8_t { kDynamicValue, kGenFunc };
 
+///////////////////////////////////////////////////////////////////////////////
+// Flag current value auxiliary structs.
+
+// The minimum atomic size we believe to generate lock free code, i.e. all
+// trivially copyable types not bigger this size generate lock free code.
+static constexpr int kMinLockFreeAtomicSize = 8;
+
+// The same as kMinLockFreeAtomicSize but maximum atomic size. As double words
+// might use two registers, we want to dispatch the logic for them.
+#if defined(ABSL_FLAGS_INTERNAL_ATOMIC_DOUBLE_WORD)
+static constexpr int kMaxLockFreeAtomicSize = 16;
+#else
+static constexpr int kMaxLockFreeAtomicSize = 8;
+#endif
+
+// We can use atomic in cases when it fits in the register, trivially copyable
+// in order to make memcpy operations.
+template <typename T>
+struct IsAtomicFlagTypeTrait {
+  static constexpr bool value =
+      (sizeof(T) <= kMaxLockFreeAtomicSize &&
+       type_traits_internal::is_trivially_copyable<T>::value);
+};
+
+// Clang does not always produce cmpxchg16b instruction when alignment of a 16
+// bytes type is not 16.
+struct alignas(16) FlagsInternalTwoWordsType {
+  int64_t first;
+  int64_t second;
+};
+
+constexpr bool operator==(const FlagsInternalTwoWordsType& that,
+                          const FlagsInternalTwoWordsType& other) {
+  return that.first == other.first && that.second == other.second;
+}
+constexpr bool operator!=(const FlagsInternalTwoWordsType& that,
+                          const FlagsInternalTwoWordsType& other) {
+  return !(that == other);
+}
+
+constexpr int64_t SmallAtomicInit() { return 0xababababababababll; }
+
+template <typename T, typename S = void>
+struct BestAtomicType {
+  using type = int64_t;
+  static constexpr int64_t AtomicInit() { return SmallAtomicInit(); }
+};
+
+template <typename T>
+struct BestAtomicType<
+    T, typename std::enable_if<(kMinLockFreeAtomicSize < sizeof(T) &&
+                                sizeof(T) <= kMaxLockFreeAtomicSize),
+                               void>::type> {
+  using type = FlagsInternalTwoWordsType;
+  static constexpr FlagsInternalTwoWordsType AtomicInit() {
+    return {SmallAtomicInit(), SmallAtomicInit()};
+  }
+};
+
+struct FlagValue {
+  // Heap allocated value.
+  void* dynamic = nullptr;
+  // For some types, a copy of the current value is kept in an atomically
+  // accessible field.
+  union Atomics {
+    // Using small atomic for small types.
+    std::atomic<int64_t> small_atomic;
+    template <typename T,
+              typename K = typename std::enable_if<
+                  (sizeof(T) <= kMinLockFreeAtomicSize), void>::type>
+    int64_t load() const {
+      return small_atomic.load(std::memory_order_acquire);
+    }
+
+#if defined(ABSL_FLAGS_INTERNAL_ATOMIC_DOUBLE_WORD)
+    // Using big atomics for big types.
+    std::atomic<FlagsInternalTwoWordsType> big_atomic;
+    template <typename T, typename K = typename std::enable_if<
+                              (kMinLockFreeAtomicSize < sizeof(T) &&
+                               sizeof(T) <= kMaxLockFreeAtomicSize),
+                              void>::type>
+    FlagsInternalTwoWordsType load() const {
+      return big_atomic.load(std::memory_order_acquire);
+    }
+    constexpr Atomics()
+        : big_atomic{FlagsInternalTwoWordsType{SmallAtomicInit(),
+                                               SmallAtomicInit()}} {}
+#else
+    constexpr Atomics() : small_atomic{SmallAtomicInit()} {}
+#endif
+  };
+  Atomics atomics{};
+};
+
+///////////////////////////////////////////////////////////////////////////////
+// Flag callback auxiliary structs.
+
 // Signature for the mutation callback used by watched Flags
 // The callback is noexcept.
 // TODO(rogeeff): add noexcept after C++17 support is added.
-using FlagCallback = void (*)();
+using FlagCallbackFunc = void (*)();
+
+struct FlagCallback {
+  FlagCallbackFunc func;
+  absl::Mutex guard;  // Guard for concurrent callback invocations.
+};
+
+///////////////////////////////////////////////////////////////////////////////
+// Flag implementation, which does not depend on flag value type.
+// The class encapsulates the Flag's data and access to it.
 
 struct DynValueDeleter {
-  void operator()(void* ptr) const { Delete(op, ptr); }
+  explicit DynValueDeleter(FlagOpFn op_arg = nullptr) : op(op_arg) {}
+  void operator()(void* ptr) const {
+    if (op != nullptr) Delete(op, ptr);
+  }
 
   const FlagOpFn op;
 };
 
-// The class encapsulates the Flag's data and safe access to it.
 class FlagImpl {
  public:
-  constexpr FlagImpl(const char* name, const char* filename,
-                     const flags_internal::FlagOpFn op,
-                     const flags_internal::FlagMarshallingOpFn marshalling_op,
-                     const HelpInitArg help,
-                     const flags_internal::FlagDfltGenFunc default_value_gen)
+  constexpr FlagImpl(const char* name, const char* filename, FlagOpFn op,
+                     FlagMarshallingOpFn marshalling_op, FlagHelpArg help,
+                     FlagDfltGenFunc default_value_gen)
       : name_(name),
         filename_(filename),
         op_(op),
         marshalling_op_(marshalling_op),
         help_(help.source),
         help_source_kind_(help.kind),
-        def_kind_(flags_internal::FlagDefaultSrcKind::kGenFunc),
+        def_kind_(FlagDefaultSrcKind::kGenFunc),
         default_src_(default_value_gen),
         data_guard_{} {}
 
@@ -237,7 +295,7 @@ class FlagImpl {
   bool IsSpecifiedOnCommandLine() const ABSL_LOCKS_EXCLUDED(*DataGuard());
   std::string DefaultValue() const ABSL_LOCKS_EXCLUDED(*DataGuard());
   std::string CurrentValue() const ABSL_LOCKS_EXCLUDED(*DataGuard());
-  void Read(void* dst, const flags_internal::FlagOpFn dst_op) const
+  void Read(void* dst, const FlagOpFn dst_op) const
       ABSL_LOCKS_EXCLUDED(*DataGuard());
   // Attempts to parse supplied `value` std::string. If parsing is successful, then
   // it replaces `dst` with the new value.
@@ -247,32 +305,30 @@ class FlagImpl {
 #ifndef NDEBUG
   template <typename T>
   void Get(T* dst) const {
-    Read(dst, &flags_internal::FlagOps<T>);
+    Read(dst, &FlagOps<T>);
   }
 #else
   template <typename T, typename std::enable_if<
-                            !flags_internal::IsAtomicFlagTypeTrait<T>::value,
-                            int>::type = 0>
+                            !IsAtomicFlagTypeTrait<T>::value, int>::type = 0>
   void Get(T* dst) const {
-    Read(dst, &flags_internal::FlagOps<T>);
+    Read(dst, &FlagOps<T>);
   }
   // Overload for `GetFlag()` for types that support lock-free reads.
-  template <typename T,
-            typename std::enable_if<
-                flags_internal::IsAtomicFlagTypeTrait<T>::value, int>::type = 0>
+  template <typename T, typename std::enable_if<IsAtomicFlagTypeTrait<T>::value,
+                                                int>::type = 0>
   void Get(T* dst) const {
-    using U = flags_internal::BestAtomicType<T>;
-    const typename U::type r = atomics_.template load<T>();
+    using U = BestAtomicType<T>;
+    const typename U::type r = value_.atomics.template load<T>();
     if (r != U::AtomicInit()) {
       std::memcpy(static_cast<void*>(dst), &r, sizeof(T));
     } else {
-      Read(dst, &flags_internal::FlagOps<T>);
+      Read(dst, &FlagOps<T>);
     }
   }
 #endif
 
   // Mutating access methods
-  void Write(const void* src, const flags_internal::FlagOpFn src_op)
+  void Write(const void* src, const FlagOpFn src_op)
       ABSL_LOCKS_EXCLUDED(*DataGuard());
   bool SetFromString(absl::string_view value, FlagSettingMode set_mode,
                      ValueSource source, std::string* err)
@@ -282,18 +338,18 @@ class FlagImpl {
   void StoreAtomic() ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard());
 
   // Interfaces to operate on callbacks.
-  void SetCallback(const flags_internal::FlagCallback mutation_callback)
+  void SetCallback(const FlagCallbackFunc mutation_callback)
       ABSL_LOCKS_EXCLUDED(*DataGuard());
   void InvokeCallback() const ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard());
 
   // Interfaces to save/restore mutable flag data
   template <typename T>
-  std::unique_ptr<flags_internal::FlagStateInterface> SaveState(
-      Flag<T>* flag) const ABSL_LOCKS_EXCLUDED(*DataGuard()) {
+  std::unique_ptr<FlagStateInterface> SaveState(Flag<T>* flag) const
+      ABSL_LOCKS_EXCLUDED(*DataGuard()) {
     T&& cur_value = flag->Get();
     absl::MutexLock l(DataGuard());
 
-    return absl::make_unique<flags_internal::FlagState<T>>(
+    return absl::make_unique<FlagState<T>>(
         flag, std::move(cur_value), modified_, on_command_line_, counter_);
   }
   bool RestoreState(const void* value, bool modified, bool on_command_line,
@@ -306,35 +362,47 @@ class FlagImpl {
       ABSL_LOCKS_EXCLUDED(*DataGuard());
 
  private:
-  // Lazy initialization of the Flag's data.
-  void Init();
-  // Ensures that the lazily initialized data is initialized,
-  // and returns pointer to the mutex guarding flags data.
+  // Ensures that `data_guard_` is initialized and returns it.
   absl::Mutex* DataGuard() const ABSL_LOCK_RETURNED((absl::Mutex*)&data_guard_);
   // Returns heap allocated value of type T initialized with default value.
   std::unique_ptr<void, DynValueDeleter> MakeInitValue() const
       ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard());
+  // Lazy initialization of the Flag's data.
+  void Init();
 
-  // Immutable Flag's data.
-  // Constant configuration for a particular flag.
-  const char* const name_;      // Flags name passed to ABSL_FLAG as second arg.
-  const char* const filename_;  // The file name where ABSL_FLAG resides.
-  const FlagOpFn op_;           // Type-specific handler.
-  const FlagMarshallingOpFn marshalling_op_;  // Marshalling ops handler.
-  const FlagHelpSrc help_;  // Help message literal or function to generate it.
+  // Immutable flag's state.
+
+  // Flags name passed to ABSL_FLAG as second arg.
+  const char* const name_;
+  // The file name where ABSL_FLAG resides.
+  const char* const filename_;
+  // Type-specific handler.
+  const FlagOpFn op_;
+  // Marshalling ops handler.
+  const FlagMarshallingOpFn marshalling_op_;
+  // Help message literal or function to generate it.
+  const FlagHelpMsg help_;
   // Indicates if help message was supplied as literal or generator func.
-  const FlagHelpSrcKind help_source_kind_;
+  const FlagHelpKind help_source_kind_;
 
   // Indicates that the Flag state is initialized.
   std::atomic<bool> inited_{false};
-  // Mutable Flag state (guarded by data_guard_).
-  // Additional bool to protect against multiple concurrent constructions
-  // of `data_guard_`.
+
+  // Mutable flag's state (guarded by `data_guard_`).
+
+  // Protects against multiple concurrent constructions of `data_guard_`.
   bool is_data_guard_inited_ = false;
-  // Has flag value been modified?
+  // Has this flag's value been modified?
   bool modified_ ABSL_GUARDED_BY(*DataGuard()) = false;
-  // Specified on command line.
+  // Has this flag been specified on command line.
   bool on_command_line_ ABSL_GUARDED_BY(*DataGuard()) = false;
+
+  // Mutation counter
+  int64_t counter_ ABSL_GUARDED_BY(*DataGuard()) = 0;
+
+  // Optional flag's callback and absl::Mutex to guard the invocations.
+  FlagCallback* callback_ ABSL_GUARDED_BY(*DataGuard()) = nullptr;
+
   // If def_kind_ == kDynamicValue, default_src_ holds a dynamically allocated
   // value.
   FlagDefaultSrcKind def_kind_ ABSL_GUARDED_BY(*DataGuard());
@@ -343,46 +411,10 @@ class FlagImpl {
   // value via SetCommandLineOptionWithMode. def_kind_ is used to distinguish
   // these two cases.
   FlagDefaultSrc default_src_ ABSL_GUARDED_BY(*DataGuard());
-  // Lazily initialized pointer to current value
-  void* cur_ ABSL_GUARDED_BY(*DataGuard()) = nullptr;
-  // Mutation counter
-  int64_t counter_ ABSL_GUARDED_BY(*DataGuard()) = 0;
-  // For some types, a copy of the current value is kept in an atomically
-  // accessible field.
-  union Atomics {
-    // Using small atomic for small types.
-    std::atomic<int64_t> small_atomic;
-    template <typename T,
-              typename K = typename std::enable_if<
-                  (sizeof(T) <= kMinLockFreeAtomicSize), void>::type>
-    int64_t load() const {
-      return small_atomic.load(std::memory_order_acquire);
-    }
 
-#if defined(ABSL_FLAGS_INTERNAL_ATOMIC_DOUBLE_WORD)
-    // Using big atomics for big types.
-    std::atomic<FlagsInternalTwoWordsType> big_atomic;
-    template <typename T, typename K = typename std::enable_if<
-                              (kMinLockFreeAtomicSize < sizeof(T) &&
-                               sizeof(T) <= kMaxLockFreeAtomicSize),
-                              void>::type>
-    FlagsInternalTwoWordsType load() const {
-      return big_atomic.load(std::memory_order_acquire);
-    }
-    constexpr Atomics()
-        : big_atomic{FlagsInternalTwoWordsType{SmallAtomicInit(),
-                                               SmallAtomicInit()}} {}
-#else
-    constexpr Atomics() : small_atomic{SmallAtomicInit()} {}
-#endif
-  };
-  Atomics atomics_{};
+  // Current Flag Value
+  FlagValue value_;
 
-  struct CallbackData {
-    FlagCallback func;
-    absl::Mutex guard;  // Guard for concurrent callback invocations.
-  };
-  CallbackData* callback_data_ ABSL_GUARDED_BY(*DataGuard()) = nullptr;
   // This is reserved space for an absl::Mutex to guard flag data. It will be
   // initialized in FlagImpl::Init via placement new.
   // We can't use "absl::Mutex data_guard_", since this class is not literal.
@@ -393,15 +425,18 @@ class FlagImpl {
   alignas(absl::Mutex) mutable char data_guard_[sizeof(absl::Mutex)];
 };
 
-// This is "unspecified" implementation of absl::Flag<T> type.
+///////////////////////////////////////////////////////////////////////////////
+// The "unspecified" implementation of Flag object parameterized by the
+// flag's value type.
+
 template <typename T>
 class Flag final : public flags_internal::CommandLineFlag {
  public:
   constexpr Flag(const char* name, const char* filename,
-                 const flags_internal::FlagMarshallingOpFn marshalling_op,
-                 const flags_internal::HelpInitArg help,
-                 const flags_internal::FlagDfltGenFunc default_value_gen)
-      : impl_(name, filename, &flags_internal::FlagOps<T>, marshalling_op, help,
+                 const FlagMarshallingOpFn marshalling_op,
+                 const FlagHelpArg help,
+                 const FlagDfltGenFunc default_value_gen)
+      : impl_(name, filename, &FlagOps<T>, marshalling_op, help,
               default_value_gen) {}
 
   T Get() const {
@@ -417,9 +452,9 @@ class Flag final : public flags_internal::CommandLineFlag {
     return std::move(u.value);
   }
 
-  void Set(const T& v) { impl_.Write(&v, &flags_internal::FlagOps<T>); }
+  void Set(const T& v) { impl_.Write(&v, &FlagOps<T>); }
 
-  void SetCallback(const flags_internal::FlagCallback mutation_callback) {
+  void SetCallback(const FlagCallbackFunc mutation_callback) {
     impl_.SetCallback(mutation_callback);
   }
 
@@ -434,7 +469,6 @@ class Flag final : public flags_internal::CommandLineFlag {
   }
   std::string DefaultValue() const override { return impl_.DefaultValue(); }
   std::string CurrentValue() const override { return impl_.CurrentValue(); }
-
   bool ValidateInputValue(absl::string_view value) const override {
     return impl_.ValidateInputValue(value);
   }
@@ -442,24 +476,20 @@ class Flag final : public flags_internal::CommandLineFlag {
   // Interfaces to save and restore flags to/from persistent state.
   // Returns current flag state or nullptr if flag does not support
   // saving and restoring a state.
-  std::unique_ptr<flags_internal::FlagStateInterface> SaveState() override {
+  std::unique_ptr<FlagStateInterface> SaveState() override {
     return impl_.SaveState(this);
   }
 
   // Restores the flag state to the supplied state object. If there is
   // nothing to restore returns false. Otherwise returns true.
-  bool RestoreState(const flags_internal::FlagState<T>& flag_state) {
+  bool RestoreState(const FlagState<T>& flag_state) {
     return impl_.RestoreState(&flag_state.cur_value_, flag_state.modified_,
                               flag_state.on_command_line_, flag_state.counter_);
   }
-
-  bool SetFromString(absl::string_view value,
-                     flags_internal::FlagSettingMode set_mode,
-                     flags_internal::ValueSource source,
-                     std::string* error) override {
+  bool SetFromString(absl::string_view value, FlagSettingMode set_mode,
+                     ValueSource source, std::string* error) override {
     return impl_.SetFromString(value, set_mode, source, error);
   }
-
   void CheckDefaultValueParsingRoundtrip() const override {
     impl_.CheckDefaultValueParsingRoundtrip();
   }
@@ -469,14 +499,10 @@ class Flag final : public flags_internal::CommandLineFlag {
 
   void Destroy() override { impl_.Destroy(); }
 
-  void Read(void* dst) const override {
-    impl_.Read(dst, &flags_internal::FlagOps<T>);
-  }
-  flags_internal::FlagOpFn TypeId() const override {
-    return &flags_internal::FlagOps<T>;
-  }
+  void Read(void* dst) const override { impl_.Read(dst, &FlagOps<T>); }
+  FlagOpFn TypeId() const override { return &FlagOps<T>; }
 
-  // Flag's data
+  // Flag's implementation with value type abstracted out.
   FlagImpl impl_;
 };
 
@@ -499,7 +525,7 @@ class FlagRegistrar {
     if (do_register) flags_internal::RegisterCommandLineFlag(flag_);
   }
 
-  FlagRegistrar& OnUpdate(flags_internal::FlagCallback cb) && {
+  FlagRegistrar& OnUpdate(FlagCallbackFunc cb) && {
     flag_->SetCallback(cb);
     return *this;
   }