From 7570fc58eb757feb6c852275f9573ba0fcbdfdc5 Mon Sep 17 00:00:00 2001
From: Eric Niebler <eniebler@fb.com>
Date: Thu, 16 Jun 2016 10:08:54 -0700
Subject: [PATCH] folly: fix clang's -Wundefined-var-template

Summary:
[temp] (14)/6:
> A function template, member function of a class template, variable template, or static data member of a class template shall be defined in every translation unit in which it is implicitly instantiated (14.7.1) unless the corresponding specialization is explicitly instantiated (14.7.2) in some translation unit; no diagnostic is required.

`-Wundefined-var-template` warns on any implicit instantiations that are needed but could not be performed because the definition is not available. In particular, for valid code, this warns on templates/temploids which have their definition and all relevant explicit instantiations tucked away in some source file (but for which no explicit instantiation declarations are provided in the relevant header file) - used a few times in folly. This seems a bad style, the static data member template should either be defined in the header file or should be explicitly instantiated in each .cpp file.

Reviewed By: igorsugak

Differential Revision: D3270439

fbshipit-source-id: bf305fde3af575a265d65a0ae0bf95db5597587f
---
 folly/Fingerprint.h                   | 14 +++++++--
 folly/SharedMutex.cpp                 |  9 +++---
 folly/SharedMutex.h                   | 31 +++++++++++++-----
 folly/detail/CacheLocality.cpp        | 16 ++--------
 folly/detail/CacheLocality.h          | 45 +++++++++++++++++----------
 folly/test/CacheLocalityBenchmark.cpp |  1 -
 folly/test/CacheLocalityTest.cpp      |  1 -
 folly/test/DeterministicSchedule.cpp  | 10 +++---
 folly/test/SharedMutexTest.cpp        |  5 ---
 9 files changed, 76 insertions(+), 56 deletions(-)

diff --git a/folly/Fingerprint.h b/folly/Fingerprint.h
index f1980ae2..c7a20fb7 100644
--- a/folly/Fingerprint.h
+++ b/folly/Fingerprint.h
@@ -51,12 +51,20 @@
 namespace folly {
 
 namespace detail {
+
 template <int BITS>
 struct FingerprintTable {
-  static const uint64_t poly[1 + (BITS-1)/64];
-  static const uint64_t table[8][256][1 + (BITS-1)/64];
+  static const uint64_t poly[1 + (BITS - 1) / 64];
+  static const uint64_t table[8][256][1 + (BITS - 1) / 64];
 };
-}  // namespace detail
+
+template <int BITS>
+const uint64_t FingerprintTable<BITS>::poly[1 + (BITS - 1) / 64] = {};
+
+template <int BITS>
+const uint64_t FingerprintTable<BITS>::table[8][256][1 + (BITS - 1) / 64] = {};
+
+} // namespace detail
 
 /**
  * Compute the Rabin fingerprint.
diff --git a/folly/SharedMutex.cpp b/folly/SharedMutex.cpp
index e7c5267e..23266b7c 100644
--- a/folly/SharedMutex.cpp
+++ b/folly/SharedMutex.cpp
@@ -16,7 +16,8 @@
 
 #include "SharedMutex.h"
 
-COMMON_CONCURRENCY_SHARED_MUTEX_DECLARE_STATIC_STORAGE(
-    folly::SharedMutexReadPriority);
-COMMON_CONCURRENCY_SHARED_MUTEX_DECLARE_STATIC_STORAGE(
-    folly::SharedMutexWritePriority);
+namespace folly {
+// Explicitly instantiate SharedMutex here:
+template class SharedMutexImpl<true>;
+template class SharedMutexImpl<false>;
+}
diff --git a/folly/SharedMutex.h b/folly/SharedMutex.h
index 15dc91e6..2d71a884 100644
--- a/folly/SharedMutex.h
+++ b/folly/SharedMutex.h
@@ -1429,16 +1429,33 @@ class SharedMutexImpl {
   }
 };
 
-#define COMMON_CONCURRENCY_SHARED_MUTEX_DECLARE_STATIC_STORAGE(type) \
-  template <>                                                        \
-  type::DeferredReaderSlot                                           \
-      type::deferredReaders[type::kMaxDeferredReaders *              \
-                            type::kDeferredSeparationFactor] = {};   \
-  template <>                                                        \
-  FOLLY_TLS uint32_t type::tls_lastTokenlessSlot = 0;
+template <
+    bool ReaderPriority,
+    typename Tag_,
+    template <typename> class Atom,
+    bool BlockImmediately>
+typename SharedMutexImpl<ReaderPriority, Tag_, Atom, BlockImmediately>::
+    DeferredReaderSlot
+        SharedMutexImpl<ReaderPriority, Tag_, Atom, BlockImmediately>::
+            deferredReaders[kMaxDeferredReaders * kDeferredSeparationFactor] =
+                {};
+
+template <
+    bool ReaderPriority,
+    typename Tag_,
+    template <typename> class Atom,
+    bool BlockImmediately>
+FOLLY_TLS uint32_t
+    SharedMutexImpl<ReaderPriority, Tag_, Atom, BlockImmediately>::
+        tls_lastTokenlessSlot = 0;
 
 typedef SharedMutexImpl<true> SharedMutexReadPriority;
 typedef SharedMutexImpl<false> SharedMutexWritePriority;
 typedef SharedMutexWritePriority SharedMutex;
 
+// Prevent the compiler from instantiating these in other translation units.
+// They are instantiated once in SharedMutex.cpp
+extern template class SharedMutexImpl<true>;
+extern template class SharedMutexImpl<false>;
+
 } // namespace folly
diff --git a/folly/detail/CacheLocality.cpp b/folly/detail/CacheLocality.cpp
index e75120d6..c62e0ac8 100644
--- a/folly/detail/CacheLocality.cpp
+++ b/folly/detail/CacheLocality.cpp
@@ -28,8 +28,6 @@
 #include <folly/Format.h>
 #include <folly/ScopeGuard.h>
 
-DECLARE_ACCESS_SPREADER_TYPE(std::atomic)
-
 namespace folly {
 namespace detail {
 
@@ -233,21 +231,11 @@ Getcpu::Func Getcpu::resolveVdsoFunc() {
 
 #ifdef FOLLY_TLS
 /////////////// SequentialThreadId
-
-template <>
-std::atomic<size_t> SequentialThreadId<std::atomic>::prevId(0);
-
-template <>
-FOLLY_TLS size_t SequentialThreadId<std::atomic>::currentId(0);
+template struct SequentialThreadId<std::atomic>;
 #endif
 
 /////////////// AccessSpreader
-
-template <>
-Getcpu::Func AccessSpreader<std::atomic>::pickGetcpuFunc() {
-  auto best = Getcpu::resolveVdsoFunc();
-  return best ? best : &FallbackGetcpuType::getcpu;
-}
+template struct AccessSpreader<std::atomic>;
 
 } // namespace detail
 } // namespace folly
diff --git a/folly/detail/CacheLocality.h b/folly/detail/CacheLocality.h
index 1ede43cb..c9baf528 100644
--- a/folly/detail/CacheLocality.h
+++ b/folly/detail/CacheLocality.h
@@ -161,6 +161,16 @@ struct SequentialThreadId {
 
   static FOLLY_TLS size_t currentId;
 };
+
+template <template <typename> class Atom>
+Atom<size_t> SequentialThreadId<Atom>::prevId(0);
+
+template <template <typename> class Atom>
+FOLLY_TLS size_t SequentialThreadId<Atom>::currentId(0);
+
+// Suppress this instantiation in other translation units. It is
+// instantiated in CacheLocality.cpp
+extern template struct SequentialThreadId<std::atomic>;
 #endif
 
 struct HashingThreadId {
@@ -277,7 +287,10 @@ struct AccessSpreader {
   static bool initialized;
 
   /// Returns the best getcpu implementation for Atom
-  static Getcpu::Func pickGetcpuFunc();
+  static Getcpu::Func pickGetcpuFunc() {
+    auto best = Getcpu::resolveVdsoFunc();
+    return best ? best : &FallbackGetcpuType::getcpu;
+  }
 
   /// Always claims to be on CPU zero, node zero
   static int degenerateGetcpu(unsigned* cpu, unsigned* node, void*) {
@@ -326,22 +339,20 @@ struct AccessSpreader {
   }
 };
 
-template <>
-Getcpu::Func AccessSpreader<std::atomic>::pickGetcpuFunc();
-
-#define DECLARE_ACCESS_SPREADER_TYPE(Atom)                                     \
-  namespace folly {                                                            \
-  namespace detail {                                                           \
-  template <>                                                                  \
-  Getcpu::Func AccessSpreader<Atom>::getcpuFunc =                              \
-      AccessSpreader<Atom>::degenerateGetcpu;                                  \
-  template <>                                                                  \
-  typename AccessSpreader<Atom>::CompactStripe                                 \
-      AccessSpreader<Atom>::widthAndCpuToStripe[129][128] = {};                \
-  template <>                                                                  \
-  bool AccessSpreader<Atom>::initialized = AccessSpreader<Atom>::initialize(); \
-  }                                                                            \
-  }
+template <template <typename> class Atom>
+Getcpu::Func AccessSpreader<Atom>::getcpuFunc =
+    AccessSpreader<Atom>::degenerateGetcpu;
+
+template <template <typename> class Atom>
+typename AccessSpreader<Atom>::CompactStripe
+    AccessSpreader<Atom>::widthAndCpuToStripe[kMaxCpus + 1][kMaxCpus] = {};
+
+template <template <typename> class Atom>
+bool AccessSpreader<Atom>::initialized = AccessSpreader<Atom>::initialize();
+
+// Suppress this instantiation in other translation units. It is
+// instantiated in CacheLocality.cpp
+extern template struct AccessSpreader<std::atomic>;
 
 } // namespace detail
 } // namespace folly
diff --git a/folly/test/CacheLocalityBenchmark.cpp b/folly/test/CacheLocalityBenchmark.cpp
index 4815cc24..81f094d9 100644
--- a/folly/test/CacheLocalityBenchmark.cpp
+++ b/folly/test/CacheLocalityBenchmark.cpp
@@ -31,7 +31,6 @@ using namespace folly::detail;
   template <typename dummy>                            \
   struct tag {};                                       \
   }                                                    \
-  DECLARE_ACCESS_SPREADER_TYPE(tag)                    \
   namespace folly {                                    \
   namespace detail {                                   \
   template <>                                          \
diff --git a/folly/test/CacheLocalityTest.cpp b/folly/test/CacheLocalityTest.cpp
index 9c326fde..33af739b 100644
--- a/folly/test/CacheLocalityTest.cpp
+++ b/folly/test/CacheLocalityTest.cpp
@@ -413,7 +413,6 @@ TEST(AccessSpreader, Simple) {
   template <typename dummy>                            \
   struct tag {};                                       \
   }                                                    \
-  DECLARE_ACCESS_SPREADER_TYPE(tag)                    \
   namespace folly {                                    \
   namespace detail {                                   \
   template <>                                          \
diff --git a/folly/test/DeterministicSchedule.cpp b/folly/test/DeterministicSchedule.cpp
index fe59cc02..f7668fc5 100644
--- a/folly/test/DeterministicSchedule.cpp
+++ b/folly/test/DeterministicSchedule.cpp
@@ -15,15 +15,17 @@
  */
 
 #include <folly/test/DeterministicSchedule.h>
+
+#include <assert.h>
+
 #include <algorithm>
 #include <list>
 #include <mutex>
 #include <random>
-#include <utility>
 #include <unordered_map>
-#include <assert.h>
+#include <utility>
 
-DECLARE_ACCESS_SPREADER_TYPE(folly::test::DeterministicAtomic)
+#include <folly/Random.h>
 
 namespace folly {
 namespace test {
@@ -139,7 +141,7 @@ int DeterministicSchedule::getRandNumber(int n) {
   if (tls_sched) {
     return tls_sched->scheduler_(n);
   }
-  return std::rand() % n;
+  return Random::rand32() % n;
 }
 
 int DeterministicSchedule::getcpu(unsigned* cpu,
diff --git a/folly/test/SharedMutexTest.cpp b/folly/test/SharedMutexTest.cpp
index 8672e665..98df9d09 100644
--- a/folly/test/SharedMutexTest.cpp
+++ b/folly/test/SharedMutexTest.cpp
@@ -42,11 +42,6 @@ typedef SharedMutexImpl<true, void, DeterministicAtomic, true>
 typedef SharedMutexImpl<false, void, DeterministicAtomic, true>
     DSharedMutexWritePriority;
 
-COMMON_CONCURRENCY_SHARED_MUTEX_DECLARE_STATIC_STORAGE(
-    DSharedMutexReadPriority);
-COMMON_CONCURRENCY_SHARED_MUTEX_DECLARE_STATIC_STORAGE(
-    DSharedMutexWritePriority);
-
 template <typename Lock>
 void runBasicTest() {
   Lock lock;
-- 
2.34.1