Commit 99f856ae authored by Rob Sherwood's avatar Rob Sherwood Committed by Facebook GitHub Bot

Changed ConstructorCallback to work with -Wglobal-constructor

Summary:
The initial implementation of ConstructorCallback used a global (pre-main)
initialization which was deemed safe.  While likely still safe, it turns out
that when trying to land diffs that used ConstructorCallback, many code bases
compile with -Wglobal-constructor which explicitly denies this type of
initialization (even if it is safe).

Re-worked the code to prevent any non-zero initialization and still maintain
the property that we don't lock unless someone is registering a callback.

Reviewed By: bschlinker

Differential Revision: D28395301

fbshipit-source-id: 2cdd82189410b3ccea1c892560d0d0cb5b6abc34
parent c464335f
...@@ -17,11 +17,12 @@ ...@@ -17,11 +17,12 @@
#pragma once #pragma once
#include <array> #include <array>
#include <atomic> #include <atomic>
#include <iterator>
#include <memory>
#include <stdexcept> #include <stdexcept>
#include <folly/Format.h> #include <folly/Format.h>
#include <folly/Function.h> #include <folly/Function.h>
#include <folly/Optional.h>
#include <folly/SharedMutex.h> #include <folly/SharedMutex.h>
namespace folly { namespace folly {
...@@ -102,10 +103,10 @@ class ConstructorCallback { ...@@ -102,10 +103,10 @@ class ConstructorCallback {
* We don't need the full lock here, just the atomic int to tell us * We don't need the full lock here, just the atomic int to tell us
* how far into the array to go/how many callbacks are registered * how far into the array to go/how many callbacks are registered
* *
* NOTE that nCBs > 0 will always imply that callbacks_ is non-nullopt * NOTE that nCBs > 0 will always imply that callbacks_ is non-nullptr
*/ */
for (int i = 0; i < nCBs; i++) { for (int i = 0; i < nCBs; i++) {
This::callbacks_.value()[i](t); (*This::callbacks_)[i](t);
} }
} }
...@@ -128,16 +129,16 @@ class ConstructorCallback { ...@@ -128,16 +129,16 @@ class ConstructorCallback {
static void addNewConstructorCallback(NewConstructorCallback cb) { static void addNewConstructorCallback(NewConstructorCallback cb) {
std::lock_guard<SharedMutex> g(This::getMutex()); std::lock_guard<SharedMutex> g(This::getMutex());
auto idx = nConstructorCallbacks_.load(std::memory_order_acquire); auto idx = nConstructorCallbacks_.load(std::memory_order_acquire);
if (!callbacks_) { if (callbacks_ == nullptr) {
// initialize the array if unallocated // initialize the array if unallocated
callbacks_.emplace( callbacks_ = std::make_unique<
std::array<This::NewConstructorCallback, MaxCallbacks>()); std::array<This::NewConstructorCallback, MaxCallbacks>>();
} }
if (idx >= callbacks_.value().size()) { if (idx >= (*callbacks_).size()) {
throw std::length_error( throw std::length_error(
folly::sformat("Too many callbacks - max {}", MaxCallbacks)); folly::sformat("Too many callbacks - max {}", MaxCallbacks));
} }
callbacks_.value()[idx] = std::move(cb); (*callbacks_)[idx] = std::move(cb);
// Only increment nConstructorCallbacks_ after fully initializing the array // Only increment nConstructorCallbacks_ after fully initializing the array
// entry. This step makes the new array entry visible to other threads. // entry. This step makes the new array entry visible to other threads.
nConstructorCallbacks_.store(idx + 1, std::memory_order_release); nConstructorCallbacks_.store(idx + 1, std::memory_order_release);
...@@ -145,7 +146,7 @@ class ConstructorCallback { ...@@ -145,7 +146,7 @@ class ConstructorCallback {
private: private:
// allocate an array internal to function to avoid init() races // allocate an array internal to function to avoid init() races
static folly::Optional<std::array<NewConstructorCallback, MaxCallbacks>> static std::unique_ptr<std::array<NewConstructorCallback, MaxCallbacks>>
callbacks_; callbacks_;
static folly::SharedMutex& getMutex(); static folly::SharedMutex& getMutex();
static std::atomic<int> nConstructorCallbacks_; static std::atomic<int> nConstructorCallbacks_;
...@@ -162,9 +163,9 @@ folly::SharedMutex& ConstructorCallback<T, MaxCallbacks>::getMutex() { ...@@ -162,9 +163,9 @@ folly::SharedMutex& ConstructorCallback<T, MaxCallbacks>::getMutex() {
} }
template <class T, std::size_t MaxCallbacks> template <class T, std::size_t MaxCallbacks>
folly::Optional<std::array< std::unique_ptr<std::array<
typename ConstructorCallback<T, MaxCallbacks>::NewConstructorCallback, typename ConstructorCallback<T, MaxCallbacks>::NewConstructorCallback,
MaxCallbacks>> MaxCallbacks>>
ConstructorCallback<T, MaxCallbacks>::callbacks_{folly::none}; ConstructorCallback<T, MaxCallbacks>::callbacks_{nullptr};
} // namespace folly } // namespace folly
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment