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 847de408cbb2358bbb664d971cc33e73b0b2ef7f
Change-Id: I46ee1f01e7d4e2b125c2c534c6550824ba1de4c0
diff --git a/.waf-tools/default-compiler-flags.py b/.waf-tools/default-compiler-flags.py
index 4aa9e9b..7746db9 100644
--- a/.waf-tools/default-compiler-flags.py
+++ b/.waf-tools/default-compiler-flags.py
@@ -158,7 +158,6 @@
'-Wpedantic',
'-Wenum-conversion',
'-Wextra-semi',
- '-Wnon-virtual-dtor',
'-Wno-unused-parameter',
]
__linkFlags = ['-Wl,-O1']
diff --git a/daemon/common/config-file.hpp b/daemon/common/config-file.hpp
index fac8da8..06cd70b 100644
--- a/daemon/common/config-file.hpp
+++ b/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 @@
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>() +
diff --git a/daemon/face/ethernet-factory.cpp b/daemon/face/ethernet-factory.cpp
index ee887c0..f7fb63f 100644
--- a/daemon/face/ethernet-factory.cpp
+++ b/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 @@
: 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 @@
}
}
- // 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 @@
// new face: register with forwarding
this->addFace(face);
}
+
return face;
}
@@ -366,9 +367,9 @@
// 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);
diff --git a/daemon/face/internal-transport.hpp b/daemon/face/internal-transport.hpp
index 886af5c..4e77b67 100644
--- a/daemon/face/internal-transport.hpp
+++ b/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 @@
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 @@
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
{
diff --git a/daemon/face/network-predicate.cpp b/daemon/face/network-predicate.cpp
index 798ceeb..85c219a 100644
--- a/daemon/face/network-predicate.cpp
+++ b/daemon/face/network-predicate.cpp
@@ -38,8 +38,6 @@
this->clear();
}
-NetworkPredicateBase::~NetworkPredicateBase() = default;
-
void
NetworkPredicateBase::clear()
{
diff --git a/daemon/face/network-predicate.hpp b/daemon/face/network-predicate.hpp
index 965ec88..815afcc 100644
--- a/daemon/face/network-predicate.hpp
+++ b/daemon/face/network-predicate.hpp
@@ -40,9 +40,6 @@
public:
NetworkPredicateBase();
- virtual
- ~NetworkPredicateBase();
-
/**
* \brief Set the whitelist to "*" and clear the blacklist.
*/
@@ -59,6 +56,25 @@
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 @@
* 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 @@
* 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
diff --git a/daemon/face/udp-factory.cpp b/daemon/face/udp-factory.cpp
index c4baac2..2c22c4c 100644
--- a/daemon/face/udp-factory.cpp
+++ b/daemon/face/udp-factory.cpp
@@ -51,7 +51,7 @@
: ProtocolFactory(params)
{
m_netifAddConn = netmon->onInterfaceAdded.connect([this] (const auto& netif) {
- this->applyMcastConfigToNetif(netif);
+ applyMcastConfigToNetif(netif);
});
}
@@ -238,10 +238,10 @@
}
}
- // 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 @@
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 @@
// 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);
diff --git a/daemon/fw/retx-suppression-exponential.hpp b/daemon/fw/retx-suppression-exponential.hpp
index 82dec6c..6935bfd 100644
--- a/daemon/fw/retx-suppression-exponential.hpp
+++ b/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 @@
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 @@
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;
}
diff --git a/daemon/mgmt/manager-base.hpp b/daemon/mgmt/manager-base.hpp
index 08aca11..d9b351a 100644
--- a/daemon/mgmt/manager-base.hpp
+++ b/daemon/mgmt/manager-base.hpp
@@ -53,9 +53,6 @@
using std::runtime_error::runtime_error;
};
- virtual
- ~ManagerBase();
-
const std::string&
getModule() const
{
@@ -71,6 +68,11 @@
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,
diff --git a/daemon/table/name-tree-iterator.hpp b/daemon/table/name-tree-iterator.hpp
index 2399826..1b97c92 100644
--- a/daemon/table/name-tree-iterator.hpp
+++ b/daemon/table/name-tree-iterator.hpp
@@ -124,7 +124,8 @@
std::ostream&
operator<<(std::ostream& os, const Iterator& i);
-/** \brief Enumeration operation implementation.
+/**
+ * \brief Enumeration operation implementation.
*/
class EnumerationImpl
{
@@ -132,18 +133,19 @@
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 @@
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 @@
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 @@
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>;
diff --git a/tests/daemon/fw/topology-tester.cpp b/tests/daemon/fw/topology-tester.cpp
index a758ee2..a7ff33a 100644
--- a/tests/daemon/fw/topology-tester.cpp
+++ b/tests/daemon/fw/topology-tester.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,
@@ -125,7 +125,7 @@
m_clientTransport->connectToForwarder(m_forwarderTransport);
}
-class TopologyBareLink::Observer : public face::InternalTransportBase
+class TopologyBareLink::Observer final : public face::InternalTransportBase
{
public:
explicit
diff --git a/tests/daemon/fw/topology-tester.hpp b/tests/daemon/fw/topology-tester.hpp
index 35c1fd5..e82705c 100644
--- a/tests/daemon/fw/topology-tester.hpp
+++ b/tests/daemon/fw/topology-tester.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,
@@ -110,7 +110,7 @@
bool m_isUp = true;
time::nanoseconds m_delay;
- class ReceiveProxy : public face::InternalTransportBase
+ class ReceiveProxy final : public face::InternalTransportBase
{
public:
using Callback = std::function<void(const Block&)>;
diff --git a/tests/daemon/mgmt/manager-base.t.cpp b/tests/daemon/mgmt/manager-base.t.cpp
index ea7aea5..cf24989 100644
--- a/tests/daemon/mgmt/manager-base.t.cpp
+++ b/tests/daemon/mgmt/manager-base.t.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,
@@ -48,7 +48,7 @@
}
};
-class DummyManager : public ManagerBase
+class DummyManager final : public ManagerBase
{
public:
explicit
@@ -59,7 +59,7 @@
private:
ndn::mgmt::Authorization
- makeAuthorization(const std::string& verb) override
+ makeAuthorization(const std::string& verb) final
{
return [] (const Name& prefix, const Interest& interest,
const ndn::mgmt::ControlParameters* params,