From: Mukund Sivaraman Date: Wed, 11 Sep 2013 19:21:59 +0000 (+0530) Subject: [3112] Don't use boost::optional in libdns++ X-Git-Tag: bind10-1.2.0beta1-release~205^2^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d8a6c3dd5baed22bb4b1b8729ffc9ddcece48924;p=thirdparty%2Fkea.git [3112] Don't use boost::optional in libdns++ This has the side-effect of requiring additional copies of RRTTL to be constructed and is less optimal. See the ticket on why boost::optional is problematic. --- diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc index 5c96563975..156e0442a3 100644 --- a/src/lib/dns/master_loader.cc +++ b/src/lib/dns/master_loader.cc @@ -224,15 +224,10 @@ private: // after the RR class below. } - const MaybeRRClass rrclass = - RRClass::createFromText(rrparam_token.getString()); + boost::scoped_ptr rrclass + (RRClass::createFromText(rrparam_token.getString())); if (rrclass) { - // FIXME: The following code re-parses the rrparam_token to - // make an RRClass instead of using the MaybeRRClass above, - // because some old versions of boost::optional (that we - // still want to support) have a bug (see trac #2593). This - // workaround should be removed at some point in the future. - if (RRClass(rrparam_token.getString()) != zone_class_) { + if (*rrclass != zone_class_) { isc_throw(InternalException, "Class mismatch: " << *rrclass << " vs. " << zone_class_); } @@ -296,9 +291,9 @@ private: // We use the factory version instead of RRTTL constructor as we // need to expect cases where ttl_txt does not actually represent a TTL // but an RR class or type. - const MaybeRRTTL maybe_ttl = RRTTL::createFromText(ttl_txt); - if (maybe_ttl) { - current_ttl_ = maybe_ttl; + RRTTL* rrttl = RRTTL::createFromText(ttl_txt); + if (rrttl) { + current_ttl_.reset(rrttl); limitTTL(*current_ttl_, false); return (true); } @@ -329,7 +324,7 @@ private: dynamic_cast(*rdata). getMinimum(); setDefaultTTL(RRTTL(ttl_val), true); - current_ttl_ = *default_ttl_; + current_ttl_.reset(new RRTTL(*default_ttl_)); } else { // On catching the exception we'll try to reach EOL again, // so we need to unget it now. @@ -338,7 +333,7 @@ private: "no TTL specified; load rejected"); } } else if (!explicit_ttl && default_ttl_) { - current_ttl_ = *default_ttl_; + current_ttl_.reset(new RRTTL(*default_ttl_)); } else if (!explicit_ttl && warn_rfc1035_ttl_) { // Omitted (class and) TTL values are default to the last // explicitly stated values (RFC 1035, Sec. 5.1). @@ -407,8 +402,10 @@ private: boost::scoped_ptr default_ttl_; // Default TTL of RRs used when // unspecified. If NULL no default // is known. - MaybeRRTTL current_ttl_; // The TTL used most recently. Initially unset. - // Once set always stores a valid RRTTL. + boost::scoped_ptr current_ttl_; // The TTL used most recently. + // Initially unset. Once set + // always stores a valid + // RRTTL. const MasterLoader::Options options_; const std::string master_file_; std::string string_token_; diff --git a/src/lib/dns/rrclass-placeholder.h b/src/lib/dns/rrclass-placeholder.h index 89dc49d8c3..fe83b13df5 100644 --- a/src/lib/dns/rrclass-placeholder.h +++ b/src/lib/dns/rrclass-placeholder.h @@ -35,16 +35,6 @@ namespace dns { // forward declarations class AbstractMessageRenderer; -class RRClass; // forward declaration to define MaybeRRClass. - -/// \brief A shortcut for a compound type to represent RRClass-or-not. -/// -/// A value of this type can be interpreted in a boolean context, whose -/// value is \c true if and only if it contains a valid RRClass object. -/// And, if it contains a valid RRClass object, its value is accessible -/// using \c operator*, just like a bare pointer to \c RRClass. -typedef boost::optional MaybeRRClass; - /// /// \brief A standard DNS module exception that is thrown if an RRClass object /// is being constructed from an unrecognized string. @@ -148,6 +138,12 @@ public: /// /// \param buffer A buffer storing the wire format data. explicit RRClass(isc::util::InputBuffer& buffer); + /// \brief Copy constructor. + /// + /// This constructor never throws an exception. + /// + /// \param other The RRClass to copy from. + RRClass(const RRClass& other) : classcode_(other.classcode_) {} /// A separate factory of RRClass from text. /// @@ -163,12 +159,8 @@ public: /// RRClass(const std::string&) constructor. /// /// If the given text represents a valid RRClass, it returns a - /// \c MaybeRRClass object that stores a corresponding \c RRClass - /// object, which is accessible via \c operator*(). In this case - /// the returned object will be interpreted as \c true in a boolean - /// context. If the given text does not represent a valid RRClass, - /// it returns a \c MaybeRRClass object which is interpreted as - /// \c false in a boolean context. + /// pointer to a new \c RRClass object. If the given text does not + /// represent a valid RRClass, it returns \c NULL. /// /// One main purpose of this function is to minimize the overhead /// when the given text does not represent a valid RR class. For @@ -183,9 +175,9 @@ public: /// This function never throws the \c InvalidRRClass exception. /// /// \param class_str A string representation of the \c RRClass. - /// \return A MaybeRRClass object either storing an RRClass object - /// for the given text or a \c false value. - static MaybeRRClass createFromText(const std::string& class_str); + /// \return A new RRClass object for the given text or a \c NULL + /// value. + static RRClass* createFromText(const std::string& class_str); /// /// We use the default copy constructor intentionally. diff --git a/src/lib/dns/rrclass.cc b/src/lib/dns/rrclass.cc index 76a2e7349c..81c307c547 100644 --- a/src/lib/dns/rrclass.cc +++ b/src/lib/dns/rrclass.cc @@ -59,14 +59,14 @@ RRClass::toWire(AbstractMessageRenderer& renderer) const { renderer.writeUint16(classcode_); } -MaybeRRClass +RRClass* RRClass::createFromText(const string& class_str) { uint16_t class_code; if (RRParamRegistry::getRegistry().textToClassCode(class_str, class_code)) { - return (MaybeRRClass(class_code)); + return (new RRClass(class_code)); } - return (MaybeRRClass()); + return (NULL); } ostream& diff --git a/src/lib/dns/rrttl.cc b/src/lib/dns/rrttl.cc index f4b02c011a..73ac2599de 100644 --- a/src/lib/dns/rrttl.cc +++ b/src/lib/dns/rrttl.cc @@ -166,13 +166,13 @@ RRTTL::RRTTL(const std::string& ttlstr) { } } -MaybeRRTTL +RRTTL* RRTTL::createFromText(const string& ttlstr) { uint32_t ttlval; if (parseTTLString(ttlstr, ttlval, NULL)) { - return (MaybeRRTTL(ttlval)); + return (new RRTTL(ttlval)); } - return (MaybeRRTTL()); + return (NULL); } RRTTL::RRTTL(InputBuffer& buffer) { diff --git a/src/lib/dns/rrttl.h b/src/lib/dns/rrttl.h index 23d57f4f73..996d6d9eac 100644 --- a/src/lib/dns/rrttl.h +++ b/src/lib/dns/rrttl.h @@ -32,16 +32,6 @@ namespace dns { // forward declarations class AbstractMessageRenderer; -class RRTTL; // forward declaration to define MaybeRRTTL - -/// \brief A shortcut for a compound type to represent RRTTL-or-not. -/// -/// A value of this type can be interpreted in a boolean context, whose -/// value is \c true if and only if it contains a valid RRTTL object. -/// And, if it contains a valid RRTTL object, its value is accessible -/// using \c operator*, just like a bare pointer to \c RRTTL. -typedef boost::optional MaybeRRTTL; - /// /// \brief A standard DNS module exception that is thrown if an RRTTL object /// is being constructed from an unrecognized string. @@ -112,6 +102,12 @@ public: /// /// \param buffer A buffer storing the wire format data. explicit RRTTL(isc::util::InputBuffer& buffer); + /// \brief Copy constructor. + /// + /// This constructor never throws an exception. + /// + /// \param other The RRTTL to copy from. + RRTTL(const RRTTL& other) : ttlval_(other.ttlval_) {} /// A separate factory of RRTTL from text. /// @@ -123,12 +119,9 @@ public: /// possible exception handling. This version is provided for such /// purpose. /// - /// If the given text represents a valid RRTTL, it returns a \c MaybeRRTTL - /// object that stores a corresponding \c RRTTL object, which is - /// accessible via \c operator*(). In this case the returned object will - /// be interpreted as \c true in a boolean context. If the given text - /// does not represent a valid RRTTL, it returns a \c MaybeRRTTL object - /// which is interpreted as \c false in a boolean context. + /// If the given text represents a valid RRTTL, it returns a pointer + /// to a new RRTTL object. If the given text does not represent a + /// valid RRTTL, it returns \c NULL.. /// /// One main purpose of this function is to minimize the overhead /// when the given text does not represent a valid RR TTL. For this @@ -142,9 +135,8 @@ public: /// This function never throws the \c InvalidRRTTL exception. /// /// \param ttlstr A string representation of the \c RRTTL. - /// \return An MaybeRRTTL object either storing an RRTTL object for - /// the given text or a \c false value. - static MaybeRRTTL createFromText(const std::string& ttlstr); + /// \return A new RRTTL object for the given text or a \c NULL value. + static RRTTL* createFromText(const std::string& ttlstr); /// //@} diff --git a/src/lib/dns/tests/rrclass_unittest.cc b/src/lib/dns/tests/rrclass_unittest.cc index 17af873c50..cfd1e83916 100644 --- a/src/lib/dns/tests/rrclass_unittest.cc +++ b/src/lib/dns/tests/rrclass_unittest.cc @@ -20,10 +20,13 @@ #include +#include + using namespace std; using namespace isc; using namespace isc::dns; using namespace isc::util; +using boost::scoped_ptr; namespace { class RRClassTest : public ::testing::Test { @@ -97,11 +100,11 @@ TEST_F(RRClassTest, toText) { } TEST_F(RRClassTest, createFromText) { - const MaybeRRClass rrclass("IN"); - EXPECT_TRUE(rrclass); - EXPECT_EQ("IN", rrclass->toText()); - EXPECT_TRUE(RRClass::createFromText("CH")); - EXPECT_FALSE(RRClass::createFromText("ZZ")); + scoped_ptr chclass(RRClass::createFromText("CH")); + EXPECT_TRUE(chclass); + + scoped_ptr zzclass(RRClass::createFromText("ZZ")); + EXPECT_FALSE(zzclass); } TEST_F(RRClassTest, toWireBuffer) { diff --git a/src/lib/dns/tests/rrttl_unittest.cc b/src/lib/dns/tests/rrttl_unittest.cc index 8897102dde..c849c444ca 100644 --- a/src/lib/dns/tests/rrttl_unittest.cc +++ b/src/lib/dns/tests/rrttl_unittest.cc @@ -26,6 +26,7 @@ using namespace std; using namespace isc; using namespace isc::dns; using namespace isc::util; +using boost::scoped_ptr; namespace { class RRTTLTest : public ::testing::Test { @@ -75,6 +76,12 @@ TEST_F(RRTTLTest, getValue) { EXPECT_EQ(0xffffffff, ttl_max.getValue()); } +TEST_F(RRTTLTest, copyConstruct) { + const RRTTL ttl1(3600); + const RRTTL ttl2(ttl1); + EXPECT_EQ(ttl1.getValue(), ttl2.getValue()); +} + TEST_F(RRTTLTest, fromText) { // Border cases EXPECT_EQ(0, RRTTL("0").getValue()); @@ -88,14 +95,14 @@ TEST_F(RRTTLTest, fromText) { } TEST_F(RRTTLTest, createFromText) { - // It returns an actual RRTT iff the given text is recognized as a + // It returns an actual RRTTL iff the given text is recognized as a // valid RR TTL. - MaybeRRTTL maybe_ttl = RRTTL::createFromText("3600"); - EXPECT_TRUE(maybe_ttl); - EXPECT_EQ(RRTTL(3600), *maybe_ttl); + scoped_ptr good_ttl(RRTTL::createFromText("3600")); + EXPECT_TRUE(good_ttl); + EXPECT_EQ(RRTTL(3600), *good_ttl); - maybe_ttl = RRTTL::createFromText("bad"); - EXPECT_FALSE(maybe_ttl); + scoped_ptr bad_ttl(RRTTL::createFromText("bad")); + EXPECT_FALSE(bad_ttl); } void