style: convention for optional reference wrapper type alias (envoypro…

…xy#9725)

Description: noticed in envoyproxy#9516 that we did not have a convention around aliasing absl::optional<std::reference_wrapper<T>>. This PR proposes one.
Risk Level: low. compiles.
Testing: compilation and test suite ran.
Docs Changes: added the convention in STYLE.md

Signed-off-by: Jose Nino <jnino@lyft.com>

STYLE.md

@@ -44,6 +44,9 @@
   * `using BarSharedPtr = std::shared_ptr<Bar>;`
   * `using BlahConstSharedPtr = std::shared_ptr<const Blah>;`
   * Regular pointers (e.g. `int* foo`) should not be type aliased.
+* `absl::optional<std::reference_wrapper<T>> is type aliased:
+  * `using FooOptRef = absl::optional<std::reference_wrapper<T>>;`
+  * `using FooOptConstRef = absl::optional<std::reference_wrapper<T>>;`
 * If move semantics are intended, prefer specifying function arguments with `&&`.
   E.g., `void onHeaders(Http::HeaderMapPtr&& headers, ...)`. The rationale for this is that it
   forces the caller to specify `std::move(...)` or pass a temporary and makes the intention at

include/envoy/api/api.h

@@ -57,7 +57,7 @@ class Api {
   /**
    * @return an optional reference to the ProcessContext
    */
-  virtual OptProcessContextRef processContext() PURE;
+  virtual ProcessContextOptRef processContext() PURE;
 };
 
 using ApiPtr = std::unique_ptr<Api>;

include/envoy/network/listen_socket.h

@@ -173,6 +173,7 @@ class Socket {
 
 using SocketPtr = std::unique_ptr<Socket>;
 using SocketSharedPtr = std::shared_ptr<Socket>;
+using SocketOptRef = absl::optional<std::reference_wrapper<Socket>>;
 
 /**
  * A socket passed to a connection. For server connections this represents the accepted socket, and

include/envoy/network/listener.h

@@ -47,7 +47,7 @@ class ListenSocketFactory {
   /**
    * @return the socket shared by worker threads if any; otherwise return null.
    */
-  virtual absl::optional<std::reference_wrapper<Socket>> sharedSocket() const PURE;
+  virtual SocketOptRef sharedSocket() const PURE;
 };
 
 using ListenSocketFactorySharedPtr = std::shared_ptr<ListenSocketFactory>;

include/envoy/server/filter_config.h

@@ -195,10 +195,10 @@ class FactoryContext : public virtual CommonFactoryContext {
   virtual Grpc::Context& grpcContext() PURE;
 
   /**
-   * @return OptProcessContextRef an optional reference to the
+   * @return ProcessContextOptRef an optional reference to the
    * process context. Will be unset when running in validation mode.
    */
-  virtual OptProcessContextRef processContext() PURE;
+  virtual ProcessContextOptRef processContext() PURE;
 
   /**
    * @return ProtobufMessage::ValidationVisitor& validation visitor for filter configuration

include/envoy/server/instance.h

@@ -196,7 +196,7 @@ class Instance {
   /**
    * @return the server-wide process context.
    */
-  virtual OptProcessContextRef processContext() PURE;
+  virtual ProcessContextOptRef processContext() PURE;
 
   /**
    * @return ThreadLocal::Instance& the thread local storage engine for the server. This is used to

include/envoy/server/process_context.h

@@ -14,6 +14,8 @@ class ProcessObject {
   virtual ~ProcessObject() = default;
 };
 
+using ProcessObjectOptRef = absl::optional<std::reference_wrapper<ProcessObject>>;
+
 /**
  * Context passed to filters to access resources from non-Envoy parts of the
  * process.
@@ -28,6 +30,6 @@ class ProcessContext {
   virtual ProcessObject& get() const PURE;
 };
 
-using OptProcessContextRef = absl::optional<std::reference_wrapper<ProcessContext>>;
+using ProcessContextOptRef = absl::optional<std::reference_wrapper<ProcessContext>>;
 
 } // namespace Envoy

include/envoy/stats/scope.h

@@ -19,9 +19,9 @@ class Histogram;
 class Scope;
 class NullGaugeImpl;
 
-using OptionalCounter = absl::optional<std::reference_wrapper<const Counter>>;
-using OptionalGauge = absl::optional<std::reference_wrapper<const Gauge>>;
-using OptionalHistogram = absl::optional<std::reference_wrapper<const Histogram>>;
+using CounterOptConstRef = absl::optional<std::reference_wrapper<const Counter>>;
+using GaugeOptConstRef = absl::optional<std::reference_wrapper<const Gauge>>;
+using HistogramOptConstRef = absl::optional<std::reference_wrapper<const Histogram>>;
 using ScopePtr = std::unique_ptr<Scope>;
 using ScopeSharedPtr = std::shared_ptr<Scope>;
 
@@ -98,20 +98,20 @@ class Scope {
    * @param The name of the stat, obtained from the SymbolTable.
    * @return a reference to a counter within the scope's namespace, if it exists.
    */
-  virtual OptionalCounter findCounter(StatName name) const PURE;
+  virtual CounterOptConstRef findCounter(StatName name) const PURE;
 
   /**
    * @param The name of the stat, obtained from the SymbolTable.
    * @return a reference to a gauge within the scope's namespace, if it exists.
    */
-  virtual OptionalGauge findGauge(StatName name) const PURE;
+  virtual GaugeOptConstRef findGauge(StatName name) const PURE;
 
   /**
    * @param The name of the stat, obtained from the SymbolTable.
    * @return a reference to a histogram within the scope's namespace, if it
    * exists.
    */
-  virtual OptionalHistogram findHistogram(StatName name) const PURE;
+  virtual HistogramOptConstRef findHistogram(StatName name) const PURE;
 
   /**
    * @return a reference to the symbol table.

source/common/api/api_impl.cc

@@ -11,7 +11,7 @@ namespace Api {
 
 Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store& store,
            Event::TimeSystem& time_system, Filesystem::Instance& file_system,
-           const OptProcessContextRef& process_context)
+           const ProcessContextOptRef& process_context)
     : thread_factory_(thread_factory), store_(store), time_system_(time_system),
       file_system_(file_system), process_context_(process_context) {}
 

source/common/api/api_impl.h

@@ -18,7 +18,7 @@ class Impl : public Api {
 public:
   Impl(Thread::ThreadFactory& thread_factory, Stats::Store& store, Event::TimeSystem& time_system,
        Filesystem::Instance& file_system,
-       const OptProcessContextRef& process_context = absl::nullopt);
+       const ProcessContextOptRef& process_context = absl::nullopt);
 
   // Api::Api
   Event::DispatcherPtr allocateDispatcher() override;
@@ -27,14 +27,14 @@ class Impl : public Api {
   Filesystem::Instance& fileSystem() override { return file_system_; }
   TimeSource& timeSource() override { return time_system_; }
   const Stats::Scope& rootScope() override { return store_; }
-  OptProcessContextRef processContext() override { return process_context_; }
+  ProcessContextOptRef processContext() override { return process_context_; }
 
 private:
   Thread::ThreadFactory& thread_factory_;
   Stats::Store& store_;
   Event::TimeSystem& time_system_;
   Filesystem::Instance& file_system_;
-  OptProcessContextRef process_context_;
+  ProcessContextOptRef process_context_;
 };
 
 } // namespace Api

source/common/network/addr_family_aware_socket_option_impl.cc

@@ -39,9 +39,8 @@ absl::optional<Address::IpVersion> getVersionFromSocket(const Socket& socket) {
   return absl::nullopt;
 }
 
-absl::optional<std::reference_wrapper<SocketOptionImpl>>
-getOptionForSocket(const Socket& socket, SocketOptionImpl& ipv4_option,
-                   SocketOptionImpl& ipv6_option) {
+SocketOptionImplOptRef getOptionForSocket(const Socket& socket, SocketOptionImpl& ipv4_option,
+                                          SocketOptionImpl& ipv6_option) {
   auto version = getVersionFromSocket(socket);
   if (!version.has_value()) {
     return absl::nullopt;

source/common/network/socket_option_impl.h

@@ -146,5 +146,7 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::con
   const std::vector<uint8_t> value_;
 };
 
+using SocketOptionImplOptRef = absl::optional<std::reference_wrapper<SocketOptionImpl>>;
+
 } // namespace Network
 } // namespace Envoy

source/common/stats/isolated_store_impl.h

@@ -28,6 +28,7 @@ template <class Base> class IsolatedStatsCache {
   using CounterAllocator = std::function<RefcountPtr<Base>(StatName name)>;
   using GaugeAllocator = std::function<RefcountPtr<Base>(StatName, Gauge::ImportMode)>;
   using HistogramAllocator = std::function<RefcountPtr<Base>(StatName, Histogram::Unit)>;
+  using BaseOptConstRef = absl::optional<std::reference_wrapper<const Base>>;
 
   IsolatedStatsCache(CounterAllocator alloc) : counter_alloc_(alloc) {}
   IsolatedStatsCache(GaugeAllocator alloc) : gauge_alloc_(alloc) {}
@@ -79,7 +80,7 @@ template <class Base> class IsolatedStatsCache {
 private:
   friend class IsolatedStoreImpl;
 
-  absl::optional<std::reference_wrapper<const Base>> find(StatName name) const {
+  BaseOptConstRef find(StatName name) const {
     auto stat = stats_.find(name);
     if (stat == stats_.end()) {
       return absl::nullopt;
@@ -113,9 +114,11 @@ class IsolatedStoreImpl : public StoreImpl {
     Histogram& histogram = histograms_.get(name, unit);
     return histogram;
   }
-  OptionalCounter findCounter(StatName name) const override { return counters_.find(name); }
-  OptionalGauge findGauge(StatName name) const override { return gauges_.find(name); }
-  OptionalHistogram findHistogram(StatName name) const override { return histograms_.find(name); }
+  CounterOptConstRef findCounter(StatName name) const override { return counters_.find(name); }
+  GaugeOptConstRef findGauge(StatName name) const override { return gauges_.find(name); }
+  HistogramOptConstRef findHistogram(StatName name) const override {
+    return histograms_.find(name);
+  }
 
   // Stats::Store
   std::vector<CounterSharedPtr> counters() const override { return counters_.toVector(); }

source/common/stats/scope_prefixer.cc

@@ -44,11 +44,13 @@ Histogram& ScopePrefixer::histogramFromStatName(StatName name, Histogram::Unit u
   return scope_.histogramFromStatName(StatName(stat_name_storage.get()), unit);
 }
 
-OptionalCounter ScopePrefixer::findCounter(StatName name) const { return scope_.findCounter(name); }
+CounterOptConstRef ScopePrefixer::findCounter(StatName name) const {
+  return scope_.findCounter(name);
+}
 
-OptionalGauge ScopePrefixer::findGauge(StatName name) const { return scope_.findGauge(name); }
+GaugeOptConstRef ScopePrefixer::findGauge(StatName name) const { return scope_.findGauge(name); }
 
-OptionalHistogram ScopePrefixer::findHistogram(StatName name) const {
+HistogramOptConstRef ScopePrefixer::findHistogram(StatName name) const {
   return scope_.findHistogram(name);
 }
 

source/common/stats/scope_prefixer.h

@@ -35,9 +35,9 @@ class ScopePrefixer : public Scope {
     return histogramFromStatName(storage.statName(), unit);
   }
 
-  OptionalCounter findCounter(StatName name) const override;
-  OptionalGauge findGauge(StatName name) const override;
-  OptionalHistogram findHistogram(StatName name) const override;
+  CounterOptConstRef findCounter(StatName name) const override;
+  GaugeOptConstRef findGauge(StatName name) const override;
+  HistogramOptConstRef findHistogram(StatName name) const override;
 
   const SymbolTable& constSymbolTable() const override { return scope_.constSymbolTable(); }
   SymbolTable& symbolTable() override { return scope_.symbolTable(); }

source/common/stats/stat_merger.cc

@@ -35,7 +35,7 @@ void StatMerger::mergeGauges(const Protobuf::Map<std::string, uint64_t>& gauges)
 
     StatNameManagedStorage storage(gauge.first, temp_scope_->symbolTable());
     StatName stat_name = storage.statName();
-    OptionalGauge gauge_opt = temp_scope_->findGauge(stat_name);
+    GaugeOptConstRef gauge_opt = temp_scope_->findGauge(stat_name);
 
     Gauge::ImportMode import_mode = Gauge::ImportMode::Uninitialized;
     if (gauge_opt) {

source/common/stats/thread_local_store.cc

@@ -512,15 +512,15 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name,
   return **central_ref;
 }
 
-OptionalCounter ThreadLocalStoreImpl::ScopeImpl::findCounter(StatName name) const {
+CounterOptConstRef ThreadLocalStoreImpl::ScopeImpl::findCounter(StatName name) const {
   return findStatLockHeld<Counter>(name, central_cache_->counters_);
 }
 
-OptionalGauge ThreadLocalStoreImpl::ScopeImpl::findGauge(StatName name) const {
+GaugeOptConstRef ThreadLocalStoreImpl::ScopeImpl::findGauge(StatName name) const {
   return findStatLockHeld<Gauge>(name, central_cache_->gauges_);
 }
 
-OptionalHistogram ThreadLocalStoreImpl::ScopeImpl::findHistogram(StatName name) const {
+HistogramOptConstRef ThreadLocalStoreImpl::ScopeImpl::findHistogram(StatName name) const {
   auto iter = central_cache_->histograms_.find(name);
   if (iter == central_cache_->histograms_.end()) {
     return absl::nullopt;

source/common/stats/thread_local_store.h

@@ -182,8 +182,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   const SymbolTable& constSymbolTable() const override { return alloc_.constSymbolTable(); }
   SymbolTable& symbolTable() override { return alloc_.symbolTable(); }
   const TagProducer& tagProducer() const { return *tag_producer_; }
-  OptionalCounter findCounter(StatName name) const override {
-    OptionalCounter found_counter;
+  CounterOptConstRef findCounter(StatName name) const override {
+    CounterOptConstRef found_counter;
     Thread::LockGuard lock(lock_);
     for (ScopeImpl* scope : scopes_) {
       found_counter = scope->findCounter(name);
@@ -193,8 +193,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     }
     return absl::nullopt;
   }
-  OptionalGauge findGauge(StatName name) const override {
-    OptionalGauge found_gauge;
+  GaugeOptConstRef findGauge(StatName name) const override {
+    GaugeOptConstRef found_gauge;
     Thread::LockGuard lock(lock_);
     for (ScopeImpl* scope : scopes_) {
       found_gauge = scope->findGauge(name);
@@ -204,8 +204,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     }
     return absl::nullopt;
   }
-  OptionalHistogram findHistogram(StatName name) const override {
-    OptionalHistogram found_histogram;
+  HistogramOptConstRef findHistogram(StatName name) const override {
+    HistogramOptConstRef found_histogram;
     Thread::LockGuard lock(lock_);
     for (ScopeImpl* scope : scopes_) {
       found_histogram = scope->findHistogram(name);
@@ -306,9 +306,9 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
 
     // NOTE: The find methods assume that `name` is fully-qualified.
     // Implementations will not add the scope prefix.
-    OptionalCounter findCounter(StatName name) const override;
-    OptionalGauge findGauge(StatName name) const override;
-    OptionalHistogram findHistogram(StatName name) const override;
+    CounterOptConstRef findCounter(StatName name) const override;
+    GaugeOptConstRef findGauge(StatName name) const override;
+    HistogramOptConstRef findHistogram(StatName name) const override;
 
     template <class StatType>
     using MakeStatFn = std::function<RefcountPtr<StatType>(Allocator&, StatName name,

source/common/upstream/cluster_manager_impl.cc

@@ -1257,7 +1257,7 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool(
 
   // Note: to simplify this, we assume that the factory is only called in the scope of this
   // function. Otherwise, we'd need to capture a few of these variables by value.
-  ConnPoolsContainer::ConnPools::OptPoolRef pool =
+  ConnPoolsContainer::ConnPools::PoolOptRef pool =
       container.pools_->getPool(priority, hash_key, [&]() {
         return parent_.parent_.factory_.allocateConnPool(
             parent_.thread_local_dispatcher_, host, priority, protocol,

source/common/upstream/conn_pool_map.h

@@ -21,7 +21,7 @@ template <typename KEY_TYPE, typename POOL_TYPE> class ConnPoolMap {
 public:
   using PoolFactory = std::function<std::unique_ptr<POOL_TYPE>()>;
   using DrainedCb = std::function<void()>;
-  using OptPoolRef = absl::optional<std::reference_wrapper<POOL_TYPE>>;
+  using PoolOptRef = absl::optional<std::reference_wrapper<POOL_TYPE>>;
 
   ConnPoolMap(Event::Dispatcher& dispatcher, const HostConstSharedPtr& host,
               ResourcePriority priority);
@@ -31,7 +31,7 @@ template <typename KEY_TYPE, typename POOL_TYPE> class ConnPoolMap {
    * possible for this to fail if a limit on the number of pools allowed is reached.
    * @return The pool corresponding to `key`, or `absl::nullopt`.
    */
-  OptPoolRef getPool(KEY_TYPE key, const PoolFactory& factory);
+  PoolOptRef getPool(KEY_TYPE key, const PoolFactory& factory);
 
   /**
    * @return the number of pools.

source/common/upstream/conn_pool_map_impl.h

@@ -20,7 +20,7 @@ template <typename KEY_TYPE, typename POOL_TYPE> ConnPoolMap<KEY_TYPE, POOL_TYPE
 }
 
 template <typename KEY_TYPE, typename POOL_TYPE>
-typename ConnPoolMap<KEY_TYPE, POOL_TYPE>::OptPoolRef
+typename ConnPoolMap<KEY_TYPE, POOL_TYPE>::PoolOptRef
 ConnPoolMap<KEY_TYPE, POOL_TYPE>::getPool(KEY_TYPE key, const PoolFactory& factory) {
   Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_);
   // TODO(klarose): Consider how we will change the connection pool's configuration in the future.

source/common/upstream/priority_conn_pool_map.h

@@ -16,7 +16,7 @@ template <typename KEY_TYPE, typename POOL_TYPE> class PriorityConnPoolMap {
   using ConnPoolMapType = ConnPoolMap<KEY_TYPE, POOL_TYPE>;
   using PoolFactory = typename ConnPoolMapType::PoolFactory;
   using DrainedCb = typename ConnPoolMapType::DrainedCb;
-  using OptPoolRef = typename ConnPoolMapType::OptPoolRef;
+  using PoolOptRef = typename ConnPoolMapType::PoolOptRef;
 
   PriorityConnPoolMap(Event::Dispatcher& dispatcher, const HostConstSharedPtr& host);
   ~PriorityConnPoolMap();
@@ -26,7 +26,7 @@ template <typename KEY_TYPE, typename POOL_TYPE> class PriorityConnPoolMap {
    * is reached.
    * @return The pool corresponding to `key`, or `absl::nullopt`.
    */
-  OptPoolRef getPool(ResourcePriority priority, KEY_TYPE key, const PoolFactory& factory);
+  PoolOptRef getPool(ResourcePriority priority, KEY_TYPE key, const PoolFactory& factory);
 
   /**
    * @return the number of pools across all priorities.

source/common/upstream/priority_conn_pool_map_impl.h

@@ -19,7 +19,7 @@ template <typename KEY_TYPE, typename POOL_TYPE>
 PriorityConnPoolMap<KEY_TYPE, POOL_TYPE>::~PriorityConnPoolMap() = default;
 
 template <typename KEY_TYPE, typename POOL_TYPE>
-typename PriorityConnPoolMap<KEY_TYPE, POOL_TYPE>::OptPoolRef
+typename PriorityConnPoolMap<KEY_TYPE, POOL_TYPE>::PoolOptRef
 PriorityConnPoolMap<KEY_TYPE, POOL_TYPE>::getPool(ResourcePriority priority, KEY_TYPE key,
                                                   const PoolFactory& factory) {
   size_t index = static_cast<size_t>(priority);

source/server/config_validation/server.h

@@ -98,7 +98,7 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
   Stats::Store& stats() override { return stats_store_; }
   Grpc::Context& grpcContext() override { return grpc_context_; }
   Http::Context& httpContext() override { return http_context_; }
-  OptProcessContextRef processContext() override { return absl::nullopt; }
+  ProcessContextOptRef processContext() override { return absl::nullopt; }
   ThreadLocal::Instance& threadLocal() override { return thread_local_; }
   const LocalInfo::LocalInfo& localInfo() const override { return *local_info_; }
   TimeSource& timeSource() override { return api_->timeSource(); }

source/server/filter_chain_manager_impl.cc

@@ -115,7 +115,7 @@ ServerLifecycleNotifier& FilterChainFactoryContextImpl::lifecycleNotifier() {
   return parent_context_.lifecycleNotifier();
 }
 
-OptProcessContextRef FilterChainFactoryContextImpl::processContext() {
+ProcessContextOptRef FilterChainFactoryContextImpl::processContext() {
   return parent_context_.processContext();
 }
 
@@ -614,7 +614,7 @@ Api::Api& FactoryContextImpl::api() { return server_.api(); }
 ServerLifecycleNotifier& FactoryContextImpl::lifecycleNotifier() {
   return server_.lifecycleNotifier();
 }
-OptProcessContextRef FactoryContextImpl::processContext() { return server_.processContext(); }
+ProcessContextOptRef FactoryContextImpl::processContext() { return server_.processContext(); }
 Configuration::ServerFactoryContext& FactoryContextImpl::getServerFactoryContext() const {
   return server_.serverFactoryContext();
 }