about summary refs log tree commit diff
diff options
context:
space:
mode:
authorAbseil Team <absl-team@google.com>2020-02-10T18·18-0800
committerMark Barolak <mbar@google.com>2020-02-10T18·55-0500
commitbf78e977309c4cb946914b456404141ddac1c302 (patch)
tree3d4c99e9bccb4c0cb19a5be2eaf65bb9c81f1c34
parentd95d1567165d449e4c213ea31a15cbb112a9865f (diff)
Export of internal Abseil changes
--
803abc2dcad8b2354c988e9bf58dac4a17683832 by Gennadiy Rozental <rogeeff@google.com>:

Avoid warning when RTTI is not enabled.

PiperOrigin-RevId: 294247546

--
5a7b0b4d07d1d6e56fbb0b0ffbf4f8fcab772dbf by Derek Mauro <dmauro@google.com>:

Add a public Abseil FAQ

PiperOrigin-RevId: 294226960

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

Re-enable type mismatch check, which works in all the cases including shared libraries.

We will use RTTI in case when our hand written approximation of it reports a type mismatch. This way we can ensure that if a flag is defined in one shared object and referenced in another we do not report spurious errors.

PiperOrigin-RevId: 293905563
GitOrigin-RevId: 803abc2dcad8b2354c988e9bf58dac4a17683832
Change-Id: I1a23776d227ed2734c2e7183323786b7a95c3cc7
-rw-r--r--absl/FAQ.md144
-rw-r--r--absl/flags/BUILD.bazel1
-rw-r--r--absl/flags/config.h8
-rw-r--r--absl/flags/flag.cc7
-rw-r--r--absl/flags/flag.h15
-rw-r--r--absl/flags/flag_test.cc13
-rw-r--r--absl/flags/internal/commandlineflag.h19
-rw-r--r--absl/flags/internal/flag.cc45
-rw-r--r--absl/flags/internal/flag.h48
9 files changed, 229 insertions, 71 deletions
diff --git a/absl/FAQ.md b/absl/FAQ.md
new file mode 100644
index 000000000000..af721307c2ec
--- /dev/null
+++ b/absl/FAQ.md
@@ -0,0 +1,144 @@
+# Abseil FAQ
+
+## Is Abseil the right home for my utility library?
+
+Most often the answer to the question is "no." As both the [About
+Abseil](https://abseil.io/about/) page and our [contributing
+guidelines](https://github.com/abseil/abseil-cpp/blob/master/CONTRIBUTING.md#contribution-guidelines)
+explain, Abseil contains a variety of core C++ library code that is widely used
+at [Google](https://www.google.com/). As such, Abseil's primary purpose is to be
+used as a dependency by Google's open source C++ projects. While we do hope that
+Abseil is also useful to the C++ community at large, this added constraint also
+means that we are unlikely to accept a contribution of utility code that isn't
+already widely used by Google.
+
+## How to I set the C++ dialect used to build Abseil?
+
+The short answer is that whatever mechanism you choose, you need to make sure
+that you set this option consistently at the global level for your entire
+project. If, for example, you want to set the C++ dialect to C++17, with
+[Bazel](https://bazel/build/) as the build system and `gcc` or `clang` as the
+compiler, there several ways to do this:
+* Pass `--cxxopt=-std=c++17` on the command line (for example, `bazel build
+  --cxxopt=-std=c++17 ...`)
+* Set the environment variable `BAZEL_CXXOPTS` (for example,
+  `BAZEL_CXXOPTS=-std=c++17`)
+* Add `build --cxxopt=-std=c++17` to your [`.bazelrc`
+  file](https://docs.bazel.build/versions/master/guide.html#bazelrc)
+
+If you are using CMake as the build system, you'll need to add a line like
+`set(CMAKE_CXX_STANDARD 17)` to your top level `CMakeLists.txt` file. See the
+[CMake build
+instructions](https://github.com/abseil/abseil-cpp/blob/master/CMake/README.md)
+for more information.
+
+For a longer answer to this question and to understand why some other approaches
+don't work, see the answer to "What is ABI and why don't you recommend using a
+pre-compiled version of Abseil?"
+
+## What is ABI and why don't you recommend using a pre-compiled version of Abseil?
+
+For the purposes of this discussion, you can think of
+[ABI](https://en.wikipedia.org/wiki/Application_binary_interface) as the
+compiled representation of the interfaces in code. This is in contrast to
+[API](https://en.wikipedia.org/wiki/Application_programming_interface), which
+you can think of as the interfaces as defined by the code itself. [Abseil has a
+strong promise of API compatibility, but does not make any promise of ABI
+compatibility](https://abseil.io/about/compatibility). Let's take a look at what
+this means in practice.
+
+You might be tempted to do something like this in a
+[Bazel](https://bazel.build/) `BUILD` file:
+
+```
+# DON'T DO THIS!!!
+cc_library(
+    name = "my_library",
+    srcs = ["my_library.cc"],
+    copts = ["-std=c++17"],  # May create a mixed-mode compile!
+    deps = ["@com_google_absl//absl/strings"],
+)
+```
+
+Applying `-std=c++17` to an individual target in your `BUILD` file is going to
+compile that specific target in C++17 mode, but it isn't going to ensure the
+Abseil library is built in C++17 mode, since the Abseil library itself is a
+different build target. If your code includes an Abseil header, then your
+program may contain conflicting definitions of the same
+class/function/variable/enum, etc. As a rule, all compile options that affect
+the ABI of a program need to be applied to the entire build on a global basis.
+
+C++ has something called the [One Definition
+Rule](https://en.wikipedia.org/wiki/One_Definition_Rule) (ODR). C++ doesn't
+allow multiple definitions of the same class/function/variable/enum, etc. ODR
+violations sometimes result in linker errors, but linkers do not always catch
+violations. Uncaught ODR violations can result in strange runtime behaviors or
+crashes that can be hard to debug.
+
+If you build the Abseil library and your code using different compile options
+that affect ABI, there is a good chance you will run afoul of the One Definition
+Rule. Examples of GCC compile options that affect ABI include (but aren't
+limited to) language dialect (e.g. `-std=`), optimization level (e.g. `-O2`),
+code generation flags (e.g. `-fexceptions`), and preprocessor defines
+(e.g. `-DNDEBUG`).
+
+If you use a pre-compiled version of Abseil, (for example, from your Linux
+distribution package manager or from something like
+[vcpkg](https://github.com/microsoft/vcpkg)) you have to be very careful to
+ensure ABI compatibility across the components of your program. The only way you
+can be sure your program is going to be correct regarding ABI is to ensure
+you've used the exact same compile options as were used to build the
+pre-compiled library. This does not mean that Abseil cannot work as part of a
+Linux distribution since a knowledgeable binary packager will have ensured that
+all packages have been built with consistent compile options. This is one of the
+reasons we warn against - though do not outright reject - using Abseil as a
+pre-compiled library.
+
+Another possible way that you might afoul of ABI issues is if you accidentally
+include two versions of Abseil in your program. Multiple versions of Abseil can
+end up within the same binary if your program uses the Abseil library and
+another library also transitively depends on Abseil (resulting in what is
+sometimes called the diamond dependency problem). In cases such as this you must
+structure your build so that all libraries use the same version of Abseil.
+[Abseil's strong promise of API compatibility between
+releases](https://abseil.io/about/compatibility) means the latest "HEAD" release
+of Abseil is almost certainly the right choice if you are doing as we recommend
+and building all of your code from source.
+
+For these reasons we recommend you avoid pre-compiled code and build the Abseil
+library yourself in a consistent manner with the rest of your code.
+
+## What is "live at head" and how do I do it?
+
+From Abseil's point-of-view, "live at head" means that every Abseil source
+release (which happens on an almost daily basis) is either API compatible with
+the previous release, or comes with an automated tool that you can run over code
+to make it compatible. In practice, the need to use an automated tool is
+extremely rare. This means that upgrading from one source release to another
+should be a routine practice that can and should be performed often.
+
+We recommend you update to the latest release of Abseil as often as
+possible. Not only will you pick up bug fixes more quickly, but if you have good
+automated testing, you will catch and be able to fix any [Hyrum's
+Law](https://www.hyrumslaw.com/) dependency problems on an incremental basis
+instead of being overwhelmed by them and having difficulty isolating them if you
+wait longer between updates.
+
+If you are using the [Bazel](https://bazel.build/) build system and its
+[external dependencies](https://docs.bazel.build/versions/master/external.html)
+feature, updating the
+[`http_archive`](https://docs.bazel.build/versions/master/repo/http.html#http_archive)
+rule in your
+[`WORKSPACE`](https://docs.bazel.build/versions/master/be/workspace.html) for
+`com_google_abseil` to point to the latest release is all you need to do. You
+can commit the updated `WORKSPACE` file to your source control every time you
+update, and if you have good automated testing, you might even consider
+automating this.
+
+One thing we don't recommend is using GitHub's `master.zip` files (for example
+[https://github.com/abseil/abseil-cpp/archive/master.zip](https://github.com/abseil/abseil-cpp/archive/master.zip)),
+which are always the latest commit in the `master` branch, to implement live at
+head. Since these `master.zip` URLs are not versioned, you will lose build
+reproducibility. In addition, some build systems, including Bazel, will simply
+cache this file, which means you won't actually be updating to the latest
+release until your cache is cleared or invalidated.
diff --git a/absl/flags/BUILD.bazel b/absl/flags/BUILD.bazel
index 03833d440f55..d2ca5c6f8317 100644
--- a/absl/flags/BUILD.bazel
+++ b/absl/flags/BUILD.bazel
@@ -138,6 +138,7 @@ cc_library(
         "//absl/flags:__pkg__",
     ],
     deps = [
+        ":config",
         ":marshalling",
         "//absl/base:config",
         "//absl/base:core_headers",
diff --git a/absl/flags/config.h b/absl/flags/config.h
index fbe349611f54..001f8feaf637 100644
--- a/absl/flags/config.h
+++ b/absl/flags/config.h
@@ -56,4 +56,12 @@
 #define ABSL_FLAGS_INTERNAL_ATOMIC_DOUBLE_WORD 1
 #endif
 
+// ABSL_FLAGS_INTERNAL_HAS_RTTI macro is used for selecting if we can use RTTI
+// for flag type identification.
+#ifdef ABSL_FLAGS_INTERNAL_HAS_RTTI
+#error ABSL_FLAGS_INTERNAL_HAS_RTTI cannot be directly set
+#elif !defined(__GNUC__) || defined(__GXX_RTTI)
+#define ABSL_FLAGS_INTERNAL_HAS_RTTI 1
+#endif  // !defined(__GNUC__) || defined(__GXX_RTTI)
+
 #endif  // ABSL_FLAGS_CONFIG_H_
diff --git a/absl/flags/flag.cc b/absl/flags/flag.cc
index e67f7304c6ff..f7a457bf0ce2 100644
--- a/absl/flags/flag.cc
+++ b/absl/flags/flag.cc
@@ -22,13 +22,6 @@
 namespace absl {
 ABSL_NAMESPACE_BEGIN
 
-#ifndef NDEBUG
-#define ABSL_FLAGS_GET(T) \
-  T GetFlag(const absl::Flag<T>& flag) { return flag.Get(); }
-ABSL_FLAGS_INTERNAL_BUILTIN_TYPES(ABSL_FLAGS_GET)
-#undef ABSL_FLAGS_GET
-#endif
-
 // This global mutex protects on-demand construction of flag objects in MSVC
 // builds.
 #if defined(_MSC_VER) && !defined(__clang__)
diff --git a/absl/flags/flag.h b/absl/flags/flag.h
index 782dee2ee8df..274838cbc7f0 100644
--- a/absl/flags/flag.h
+++ b/absl/flags/flag.h
@@ -191,21 +191,6 @@ ABSL_MUST_USE_RESULT T GetFlag(const absl::Flag<T>& flag) {
   return flag.Get();
 }
 
-#ifndef NDEBUG
-// We want to validate the type mismatch between type definition and
-// declaration. The lock-free implementation does not allow us to do it,
-// so in debug builds we always use the slower implementation, which always
-// validates the type.
-
-// We currently need an external linkage for built-in types because shared
-// libraries have different addresses of flags_internal::FlagOps<T> which
-// might cause log spam when checking the same flag type.
-#define ABSL_FLAGS_INTERNAL_BUILT_IN_EXPORT(T) \
-  ABSL_MUST_USE_RESULT T GetFlag(const absl::Flag<T>& flag);
-ABSL_FLAGS_INTERNAL_BUILTIN_TYPES(ABSL_FLAGS_INTERNAL_BUILT_IN_EXPORT)
-#undef ABSL_FLAGS_INTERNAL_BUILT_IN_EXPORT
-#endif
-
 // SetFlag()
 //
 // Sets the value of an `absl::Flag` to the value `v`. Do not construct an
diff --git a/absl/flags/flag_test.cc b/absl/flags/flag_test.cc
index 6722329c1f12..6429a3e1e1b3 100644
--- a/absl/flags/flag_test.cc
+++ b/absl/flags/flag_test.cc
@@ -387,19 +387,20 @@ TEST_F(FlagTest, TestCustomUDT) {
 
 // MSVC produces link error on the type mismatch.
 // Linux does not have build errors and validations work as expected.
-#if 0  // !defined(_WIN32) && GTEST_HAS_DEATH_TEST
+#if !defined(_WIN32) && GTEST_HAS_DEATH_TEST
 
-TEST(Flagtest, TestTypeMismatchValidations) {
-  // For builtin types, GetFlag() only does validation in debug mode.
+using FlagDeathTest = FlagTest;
+
+TEST_F(FlagDeathTest, TestTypeMismatchValidations) {
   EXPECT_DEBUG_DEATH(
-      absl::GetFlag(FLAGS_mistyped_int_flag),
+      static_cast<void>(absl::GetFlag(FLAGS_mistyped_int_flag)),
       "Flag 'mistyped_int_flag' is defined as one type and declared "
       "as another");
-  EXPECT_DEATH(absl::SetFlag(&FLAGS_mistyped_int_flag, 0),
+  EXPECT_DEATH(absl::SetFlag(&FLAGS_mistyped_int_flag, 1),
                "Flag 'mistyped_int_flag' is defined as one type and declared "
                "as another");
 
-  EXPECT_DEATH(absl::GetFlag(FLAGS_mistyped_string_flag),
+  EXPECT_DEATH(static_cast<void>(absl::GetFlag(FLAGS_mistyped_string_flag)),
                "Flag 'mistyped_string_flag' is defined as one type and "
                "declared as another");
   EXPECT_DEATH(
diff --git a/absl/flags/internal/commandlineflag.h b/absl/flags/internal/commandlineflag.h
index 4bc0c12fc68c..6a0b5fad89c7 100644
--- a/absl/flags/internal/commandlineflag.h
+++ b/absl/flags/internal/commandlineflag.h
@@ -21,9 +21,11 @@
 
 #include <memory>
 #include <string>
+#include <typeinfo>
 
 #include "absl/base/config.h"
 #include "absl/base/macros.h"
+#include "absl/flags/config.h"
 #include "absl/flags/marshalling.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
@@ -41,7 +43,10 @@ enum FlagOp {
   kCopyConstruct,
   kSizeof,
   kParse,
-  kUnparse
+  kUnparse,
+#if defined(ABSL_FLAGS_INTERNAL_HAS_RTTI)
+  kRuntimeTypeId
+#endif
 };
 using FlagOpFn = void* (*)(FlagOp, const void*, void*);
 using FlagMarshallingOpFn = void* (*)(FlagOp, const void*, void*, void*);
@@ -84,6 +89,11 @@ void* FlagOps(FlagOp op, const void* v1, void* v2) {
       return nullptr;
     case kSizeof:
       return reinterpret_cast<void*>(sizeof(T));
+#if defined(ABSL_FLAGS_INTERNAL_HAS_RTTI)
+    case kRuntimeTypeId:
+      return const_cast<std::type_info*>(&typeid(T));
+      break;
+#endif
     default:
       return nullptr;
   }
@@ -146,6 +156,13 @@ inline size_t Sizeof(FlagOpFn op) {
       op(flags_internal::kSizeof, nullptr, nullptr)));
 }
 
+#if defined(ABSL_FLAGS_INTERNAL_HAS_RTTI)
+inline const std::type_info& RuntimeTypeId(FlagOpFn op) {
+  return *static_cast<const std::type_info*>(
+      op(flags_internal::kRuntimeTypeId, nullptr, nullptr));
+}
+#endif
+
 // Handle to FlagState objects. Specific flag state objects will restore state
 // of a flag produced this flag state from method CommandLineFlag::SaveState().
 class FlagStateInterface {
diff --git a/absl/flags/internal/flag.cc b/absl/flags/internal/flag.cc
index 6ce7def2d3d2..cfc0cf4d93d1 100644
--- a/absl/flags/internal/flag.cc
+++ b/absl/flags/internal/flag.cc
@@ -56,6 +56,14 @@ bool ShouldValidateFlagValue(FlagOpFn flag_type_id) {
   return true;
 }
 
+#if defined(ABSL_FLAGS_INTERNAL_HAS_RTTI)
+bool MatchRuntimeTypeId(FlagOpFn lhs_type_id, FlagOpFn rhs_type_id) {
+  return RuntimeTypeId(lhs_type_id) == RuntimeTypeId(rhs_type_id);
+}
+#else
+bool MatchRuntimeTypeId(FlagOpFn, FlagOpFn) { return true; }
+#endif
+
 // RAII helper used to temporarily unlock and relock `absl::Mutex`.
 // This is used when we need to ensure that locks are released while
 // invoking user supplied callbacks and then reacquired, since callbacks may
@@ -133,6 +141,18 @@ void FlagImpl::Destroy() {
   is_data_guard_inited_ = false;
 }
 
+void FlagImpl::AssertValidType(const flags_internal::FlagOpFn op) const {
+  // `op` is the unmarshaling operation corresponding to the declaration
+  // visibile at the call site. `op_` is the Flag's defined unmarshalling
+  // operation. They must match for this operation to be well-defined.
+  if (ABSL_PREDICT_FALSE(op != op_) && !MatchRuntimeTypeId(op, op_)) {
+    ABSL_INTERNAL_LOG(
+        FATAL,
+        absl::StrCat("Flag '", Name(),
+                     "' is defined as one type and declared as another"));
+  }
+}
+
 std::unique_ptr<void, DynValueDeleter> FlagImpl::MakeInitValue() const {
   void* res = nullptr;
   if (DefaultKind() == FlagDefaultKind::kDynamicValue) {
@@ -219,7 +239,7 @@ bool FlagImpl::RestoreState(const void* value, bool modified,
     if (counter_ == counter) return false;
   }
 
-  Write(value, op_);
+  Write(value);
 
   {
     absl::MutexLock l(DataGuard());
@@ -254,18 +274,9 @@ bool FlagImpl::TryParse(void** dst, absl::string_view value,
   return true;
 }
 
-void FlagImpl::Read(void* dst, const flags_internal::FlagOpFn dst_op) const {
+void FlagImpl::Read(void* dst) const {
   absl::ReaderMutexLock l(DataGuard());
 
-  // `dst_op` is the unmarshaling operation corresponding to the declaration
-  // visibile at the call site. `op` is the Flag's defined unmarshalling
-  // operation. They must match for this operation to be well-defined.
-  if (ABSL_PREDICT_FALSE(dst_op != op_)) {
-    ABSL_INTERNAL_LOG(
-        ERROR,
-        absl::StrCat("Flag '", Name(),
-                     "' is defined as one type and declared as another"));
-  }
   CopyConstruct(op_, value_.dynamic, dst);
 }
 
@@ -286,19 +297,9 @@ void FlagImpl::StoreAtomic() {
 #endif
 }
 
-void FlagImpl::Write(const void* src, const flags_internal::FlagOpFn src_op) {
+void FlagImpl::Write(const void* src) {
   absl::MutexLock l(DataGuard());
 
-  // `src_op` is the marshalling operation corresponding to the declaration
-  // visible at the call site. `op` is the Flag's defined marshalling operation.
-  // They must match for this operation to be well-defined.
-  if (ABSL_PREDICT_FALSE(src_op != op_)) {
-    ABSL_INTERNAL_LOG(
-        ERROR,
-        absl::StrCat("Flag '", Name(),
-                     "' is defined as one type and declared as another"));
-  }
-
   if (ShouldValidateFlagValue(op_)) {
     void* obj = Clone(op_, src);
     std::string ignored_error;
diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h
index b5471fa8a734..c6c4a2f7beb5 100644
--- a/absl/flags/internal/flag.h
+++ b/absl/flags/internal/flag.h
@@ -301,41 +301,44 @@ 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 FlagOpFn dst_op) const
-      ABSL_LOCKS_EXCLUDED(*DataGuard());
+  void Read(void* dst) const ABSL_LOCKS_EXCLUDED(*DataGuard());
   // Attempts to parse supplied `value` std::string. If parsing is successful, then
   // it replaces `dst` with the new value.
   bool TryParse(void** dst, absl::string_view value, std::string* err) const
       ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard());
 
-#ifndef NDEBUG
-  template <typename T>
-  void Get(T* dst) const {
-    Read(dst, &FlagOps<T>);
-  }
-#else
   template <typename T, typename std::enable_if<
                             !IsAtomicFlagTypeTrait<T>::value, int>::type = 0>
   void Get(T* dst) const {
-    Read(dst, &FlagOps<T>);
+    AssertValidType(&flags_internal::FlagOps<T>);
+    Read(dst);
   }
   // Overload for `GetFlag()` for types that support lock-free reads.
   template <typename T, typename std::enable_if<IsAtomicFlagTypeTrait<T>::value,
                                                 int>::type = 0>
   void Get(T* dst) const {
-    using U = BestAtomicType<T>;
-    const typename U::type r = value_.atomics.template load<T>();
+    // For flags of types which can be accessed "atomically" we want to avoid
+    // slowing down flag value access due to type validation. That's why
+    // this validation is hidden behind !NDEBUG
+#ifndef NDEBUG
+    AssertValidType(&flags_internal::FlagOps<T>);
+#endif
+    using U = flags_internal::BestAtomicType<T>;
+    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, &FlagOps<T>);
+      Read(dst);
     }
   }
-#endif
+  template <typename T>
+  void Set(const T& src) {
+    AssertValidType(&flags_internal::FlagOps<T>);
+    Write(&src);
+  }
 
   // Mutating access methods
-  void Write(const void* src, const FlagOpFn src_op)
-      ABSL_LOCKS_EXCLUDED(*DataGuard());
+  void Write(const void* src) ABSL_LOCKS_EXCLUDED(*DataGuard());
   bool SetFromString(absl::string_view value, FlagSettingMode set_mode,
                      ValueSource source, std::string* err)
       ABSL_LOCKS_EXCLUDED(*DataGuard());
@@ -383,6 +386,13 @@ class FlagImpl {
       ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard()) {
     return static_cast<FlagDefaultKind>(def_kind_);
   }
+  // Used in read/write operations to validate source/target has correct type.
+  // For example if flag is declared as absl::Flag<int> FLAGS_foo, a call to
+  // absl::GetFlag(FLAGS_foo) validates that the type of FLAGS_foo is indeed
+  // int. To do that we pass the "assumed" type id (which is deduced from type
+  // int) as an argument `op`, which is in turn is validated against the type id
+  // stored in flag object by flag definition statement.
+  void AssertValidType(const flags_internal::FlagOpFn op) const;
 
   // Immutable flag's state.
 
@@ -461,9 +471,7 @@ class Flag final : public flags_internal::CommandLineFlag {
     impl_.Get(&u.value);
     return std::move(u.value);
   }
-
-  void Set(const T& v) { impl_.Write(&v, &FlagOps<T>); }
-
+  void Set(const T& v) { impl_.Set(v); }
   void SetCallback(const FlagCallbackFunc mutation_callback) {
     impl_.SetCallback(mutation_callback);
   }
@@ -509,10 +517,10 @@ class Flag final : public flags_internal::CommandLineFlag {
 
   void Destroy() override { impl_.Destroy(); }
 
-  void Read(void* dst) const override { impl_.Read(dst, &FlagOps<T>); }
+  void Read(void* dst) const override { impl_.Read(dst); }
   FlagOpFn TypeId() const override { return &FlagOps<T>; }
 
-  // Flag's implementation with value type abstracted out.
+  // Flag's data
   FlagImpl impl_;
 };