build: disable -Wnon-virtual-dtor compiler warning

It's overkill and suffers from annoying false positives that
prevent us from applying the "protected non-virtual destructor"
idiom in several perfectly valid cases. See for instance the
GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102168

The -Wdelete-non-virtual-dtor warning (included in -Wall) is
the preferred alternative and is enough to catch the unsafe
cases without false positives.

Partially reverts 847de40

Change-Id: I46ee1f01e7d4e2b125c2c534c6550824ba1de4c0

.waf-tools/default-compiler-flags.py

@@ -158,7 +158,6 @@ def getGeneralFlags(self, conf):
         '-Wpedantic',
         '-Wenum-conversion',
         '-Wextra-semi',
-        '-Wnon-virtual-dtor',
         '-Wno-unused-parameter',
     ]
     __linkFlags = ['-Wl,-O1']

daemon/common/config-file.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2023,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -115,7 +115,7 @@ class ConfigFile : noncopyable
     auto value = node.get_value_optional<T>();
     // Unsigned logic is workaround for https://redmine.named-data.net/issues/4489
     if (value &&
-        (std::is_signed<T>() || node.get_value<std::string>().find("-") == std::string::npos)) {
+        (std::is_signed_v<T> || node.get_value<std::string>().find("-") == std::string::npos)) {
       return *value;
     }
     NDN_THROW(Error("Invalid value '" + node.get_value<std::string>() +

daemon/face/ethernet-factory.cpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2023,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -47,8 +47,8 @@ EthernetFactory::EthernetFactory(const CtorParams& params)
   : ProtocolFactory(params)
 {
   m_netifAddConn = netmon->onInterfaceAdded.connect([this] (const auto& netif) {
-    this->applyUnicastConfigToNetif(netif);
-    this->applyMcastConfigToNetif(*netif);
+    applyUnicastConfigToNetif(netif);
+    applyMcastConfigToNetif(*netif);
   });
 }
 
@@ -157,11 +157,11 @@ EthernetFactory::doProcessConfig(OptionalConfigSection configSection,
     }
   }
 
-  // Even if there's no configuration change, we still need to re-apply configuration because
-  // netifs may have changed.
-  m_unicastConfig = unicastConfig;
-  m_mcastConfig = mcastConfig;
-  this->applyConfig(context);
+  // Even if there are no configuration changes, we still need to re-apply
+  // the configuration because netifs may have changed.
+  m_unicastConfig = std::move(unicastConfig);
+  m_mcastConfig = std::move(mcastConfig);
+  applyConfig(context);
 }
 
 void
@@ -347,6 +347,7 @@ EthernetFactory::applyMcastConfigToNetif(const ndn::net::NetworkInterface& netif
     // new face: register with forwarding
     this->addFace(face);
   }
+
   return face;
 }
 
@@ -366,9 +367,9 @@ EthernetFactory::applyConfig(const FaceSystem::ConfigContext&)
 
   // create channels and multicast faces if requested by config
   for (const auto& netif : netmon->listNetworkInterfaces()) {
-    this->applyUnicastConfigToNetif(netif);
+    applyUnicastConfigToNetif(netif);
 
-    auto face = this->applyMcastConfigToNetif(*netif);
+    auto face = applyMcastConfigToNetif(*netif);
     if (face != nullptr) {
       // don't destroy face
       oldFaces.erase(face);

daemon/face/internal-transport.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2023,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -32,19 +32,21 @@
 
 namespace nfd::face {
 
-/** \brief Abstracts a transport that can be paired with another.
+/**
+ * \brief Abstracts a transport that can be paired with another.
  */
 class InternalTransportBase
 {
 public:
-  virtual
-  ~InternalTransportBase() = default;
-
   virtual void
   receivePacket(const Block& packet) = 0;
+
+protected:
+  ~InternalTransportBase() = default;
 };
 
-/** \brief Implements a forwarder-side transport that can be paired with another transport.
+/**
+ * \brief Implements a forwarder-side transport that can be paired with another transport.
  */
 class InternalForwarderTransport final : public Transport, public InternalTransportBase
 {
@@ -56,7 +58,7 @@ class InternalForwarderTransport final : public Transport, public InternalTransp
                              ndn::nfd::LinkType linkType = ndn::nfd::LINK_TYPE_POINT_TO_POINT);
 
   void
-  setPeer(InternalTransportBase* peer)
+  setPeer(InternalTransportBase* peer) noexcept
   {
     m_peer = peer;
   }
@@ -78,7 +80,8 @@ class InternalForwarderTransport final : public Transport, public InternalTransp
   InternalTransportBase* m_peer = nullptr;
 };
 
-/** \brief Implements a client-side transport that can be paired with an InternalForwarderTransport.
+/**
+ * \brief Implements a client-side transport that can be paired with an InternalForwarderTransport.
  */
 class InternalClientTransport final : public ndn::Transport, public InternalTransportBase
 {

daemon/face/network-predicate.cpp

@@ -38,8 +38,6 @@ NetworkPredicateBase::NetworkPredicateBase()
   this->clear();
 }
 
-NetworkPredicateBase::~NetworkPredicateBase() = default;
-
 void
 NetworkPredicateBase::clear()
 {

daemon/face/network-predicate.hpp

@@ -40,9 +40,6 @@ class NetworkPredicateBase : private boost::equality_comparable<NetworkPredicate
 public:
   NetworkPredicateBase();
 
-  virtual
-  ~NetworkPredicateBase();
-
   /**
    * \brief Set the whitelist to "*" and clear the blacklist.
    */
@@ -59,6 +56,25 @@ class NetworkPredicateBase : private boost::equality_comparable<NetworkPredicate
   assign(std::initializer_list<std::pair<std::string, std::string>> whitelist,
          std::initializer_list<std::pair<std::string, std::string>> blacklist);
 
+protected:
+  // Explicitly declare the following four special member functions
+  // to avoid -Wdeprecated-copy-with-dtor warnings from clang.
+
+  NetworkPredicateBase(const NetworkPredicateBase&) = delete;
+
+  NetworkPredicateBase(NetworkPredicateBase&&) = default;
+
+  NetworkPredicateBase&
+  operator=(const NetworkPredicateBase&) = delete;
+
+  NetworkPredicateBase&
+  operator=(NetworkPredicateBase&&) = default;
+
+  // NetworkPredicateBase is not supposed to be used polymorphically, so we make the destructor
+  // protected to prevent deletion of derived objects through a pointer to the base class,
+  // which would be UB when the destructor is non-virtual.
+  ~NetworkPredicateBase() = default;
+
 private:
   virtual bool
   isRuleSupported(const std::string& key) = 0;
@@ -95,7 +111,7 @@ class NetworkPredicateBase : private boost::equality_comparable<NetworkPredicate
  * ndn::net::NetworkInterface is accepted if it matches any entry in the whitelist and none of
  * the entries in the blacklist.
  */
-class NetworkInterfacePredicate : public NetworkPredicateBase
+class NetworkInterfacePredicate final : public NetworkPredicateBase
 {
 public:
   bool
@@ -117,7 +133,7 @@ class NetworkInterfacePredicate : public NetworkPredicateBase
  * 2001:db8:2::/64`) or a wildcard (`*`) that matches all IP addresses. An IP address is
  * accepted if it matches any entry in the whitelist and none of the entries in the blacklist.
  */
-class IpAddressPredicate : public NetworkPredicateBase
+class IpAddressPredicate final : public NetworkPredicateBase
 {
 public:
   bool

daemon/face/udp-factory.cpp

@@ -51,7 +51,7 @@ UdpFactory::UdpFactory(const CtorParams& params)
   : ProtocolFactory(params)
 {
   m_netifAddConn = netmon->onInterfaceAdded.connect([this] (const auto& netif) {
-    this->applyMcastConfigToNetif(netif);
+    applyMcastConfigToNetif(netif);
   });
 }
 
@@ -238,10 +238,10 @@ UdpFactory::doProcessConfig(OptionalConfigSection configSection,
     }
   }
 
-  // Even if there's no configuration change, we still need to re-apply configuration because
-  // netifs may have changed.
-  m_mcastConfig = mcastConfig;
-  this->applyMcastConfig(context);
+  // Even if there are no configuration changes, we still need to re-apply
+  // the configuration because netifs may have changed.
+  m_mcastConfig = std::move(mcastConfig);
+  applyMcastConfig(context);
 }
 
 void
@@ -434,7 +434,7 @@ UdpFactory::applyMcastConfigToNetif(const shared_ptr<const net::NetworkInterface
     NFD_LOG_DEBUG("Not creating multicast faces on " << netif->getName() << ": no viable IP address");
     // keep an eye on new addresses
     m_netifConns[netif->getIndex()].addrAddConn =
-      netif->onAddressAdded.connect([=] (auto&&...) { this->applyMcastConfigToNetif(netif); });
+      netif->onAddressAdded.connect([=] (auto&&...) { applyMcastConfigToNetif(netif); });
     return {};
   }
 
@@ -464,7 +464,7 @@ UdpFactory::applyMcastConfig(const FaceSystem::ConfigContext&)
 
   // create faces if requested by config
   for (const auto& netif : netmon->listNetworkInterfaces()) {
-    auto facesToKeep = this->applyMcastConfigToNetif(netif);
+    auto facesToKeep = applyMcastConfigToNetif(netif);
     for (const auto& face : facesToKeep) {
       // don't destroy face
       facesToClose.erase(face);

daemon/fw/retx-suppression-exponential.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022,  Regents of the University of California,
+ * Copyright (c) 2014-2023,  Regents of the University of California,
  *                           Arizona Board of Regents,
  *                           Colorado State University,
  *                           University Pierre & Marie Curie, Sorbonne University,
@@ -51,19 +51,22 @@ class RetxSuppressionExponential
                              Duration maxInterval = DEFAULT_MAX_INTERVAL,
                              float multiplier = DEFAULT_MULTIPLIER);
 
-  /** \brief Determines whether Interest is a retransmission per pit entry
-   *         and if so, whether it shall be forwarded or suppressed.
+  /**
+   * \brief Determines whether Interest is a retransmission per PIT entry
+   *        and if so, whether it shall be forwarded or suppressed.
    */
   RetxSuppressionResult
   decidePerPitEntry(pit::Entry& pitEntry);
 
-  /** \brief Determines whether Interest is a retransmission per upstream
-   *         and if so, whether it shall be forwarded or suppressed.
+  /**
+   * \brief Determines whether Interest is a retransmission per upstream
+   *        and if so, whether it shall be forwarded or suppressed.
    */
   RetxSuppressionResult
   decidePerUpstream(pit::Entry& pitEntry, Face& outFace);
 
-  /** \brief Increment the suppression interval for out record.
+  /**
+   * \brief Increment the suppression interval for an out-record.
    */
   void
   incrementIntervalForOutRecord(pit::OutRecord& outRecord);
@@ -75,8 +78,8 @@ class RetxSuppressionExponential
   friend std::ostream&
   operator<<(std::ostream& os, const RetxSuppressionExponential& retxSupp)
   {
-    return os << "RetxSuppressionExponential initial-interval=" << retxSupp.m_initialInterval
-              << " max-interval=" << retxSupp.m_maxInterval
+    return os << "RetxSuppressionExponential initial-interval=" << retxSupp.m_initialInterval.count()
+              << " max-interval=" << retxSupp.m_maxInterval.count()
               << " multiplier=" << retxSupp.m_multiplier;
   }
 

daemon/mgmt/manager-base.hpp

@@ -53,9 +53,6 @@ class ManagerBase : noncopyable
     using std::runtime_error::runtime_error;
   };
 
-  virtual
-  ~ManagerBase();
-
   const std::string&
   getModule() const
   {
@@ -71,6 +68,11 @@ class ManagerBase : noncopyable
   ManagerBase(std::string_view module, Dispatcher& dispatcher,
               CommandAuthenticator& authenticator);
 
+  // ManagerBase is not supposed to be used polymorphically, so we make the destructor
+  // protected to prevent deletion of derived objects through a pointer to the base class,
+  // which would be UB when the destructor is non-virtual.
+  ~ManagerBase();
+
 NFD_PUBLIC_WITH_TESTS_ELSE_PROTECTED: // registrations to the dispatcher
   // difference from mgmt::ControlCommand: accepts nfd::ControlParameters
   using ControlCommandHandler = std::function<void(const ControlCommand& command,

daemon/table/name-tree-iterator.hpp

@@ -124,26 +124,28 @@ class Iterator : public boost::forward_iterator_helper<Iterator, const Entry>
 std::ostream&
 operator<<(std::ostream& os, const Iterator& i);
 
-/** \brief Enumeration operation implementation.
+/**
+ * \brief Enumeration operation implementation.
  */
 class EnumerationImpl
 {
 public:
   explicit
   EnumerationImpl(const NameTree& nt);
 
-  virtual
-  ~EnumerationImpl() = default;
-
   virtual void
   advance(Iterator& i) = 0;
 
+protected:
+  ~EnumerationImpl() = default;
+
 protected:
   const NameTree& nt;
   const Hashtable& ht;
 };
 
-/** \brief Full enumeration implementation.
+/**
+ * \brief Full enumeration implementation.
  */
 class FullEnumerationImpl final : public EnumerationImpl
 {
@@ -157,10 +159,11 @@ class FullEnumerationImpl final : public EnumerationImpl
   EntrySelector m_pred;
 };
 
-/** \brief Partial enumeration implementation.
+/**
+ * \brief Partial enumeration implementation.
  *
- *  Iterator::m_ref should be initialized to subtree root.
- *  Iterator::m_state LSB indicates whether to visit children of m_entry.
+ * Iterator::m_ref should be initialized to subtree root.
+ * Iterator::m_state LSB indicates whether to visit children of m_entry.
  */
 class PartialEnumerationImpl final : public EnumerationImpl
 {
@@ -174,9 +177,10 @@ class PartialEnumerationImpl final : public EnumerationImpl
   EntrySubTreeSelector m_pred;
 };
 
-/** \brief Partial enumeration implementation.
+/**
+ * \brief Partial enumeration implementation.
  *
- *  Iterator::m_ref should be initialized to longest prefix matched entry.
+ * Iterator::m_ref should be initialized to longest prefix matched entry.
  */
 class PrefixMatchImpl final : public EnumerationImpl
 {
@@ -191,10 +195,11 @@ class PrefixMatchImpl final : public EnumerationImpl
   EntrySelector m_pred;
 };
 
-/** \brief A Forward Range of name tree entries.
+/**
+ * \brief A forward range of name tree entries.
  *
- *  This type has .begin() and .end() methods which return Iterator.
- *  This type is usable with range-based for.
+ * This type has `.begin()` and `.end()` methods which return Iterator.
+ * This type is usable with range-based for loops.
  */
 using Range = boost::iterator_range<Iterator>;