[4231] Addressed code comments

diff --git a/src/lib/eval/evaluate.cc b/src/lib/eval/evaluate.cc
index a70ee2c6ed53805decf9cd25a238ccbff12b26da..dd5e0af68de766d1b967268136765918eb1c5660 100644 (file)
--- a/src/lib/eval/evaluate.cc
+++ b/src/lib/eval/evaluate.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -27,14 +27,7 @@ bool evaluate(const Expression& expr, const Pkt& pkt) {
         isc_throw(EvalBadStack, "Incorrect stack order. Expected exactly "
                   "1 value at the end of evaluatuion, got " << values.size());
     }
-    if (values.top() == "false") {
-        return (false);
-    } else if (values.top() == "true") {
-        return (true);
-    } else {
-        isc_throw(EvalTypeError, "Incorrect evaluation type. Expected "
-                  "\"false\" or \"true\", got \"" << values.top() << "\"");
-    }
+    return (Token::toBool(values.top()));
 }
 
 }; // end of isc::dhcp namespace

diff --git a/src/lib/eval/lexer.cc b/src/lib/eval/lexer.cc
index 99843f281423aed4f54706d6bb79a00e25259e8a..b9fbae02b50af59dc7b71a93ac2d61a59c0bc98f 100644 (file)
--- a/src/lib/eval/lexer.cc
+++ b/src/lib/eval/lexer.cc
@@ -665,7 +665,7 @@ goto find_rule; \
 #define YY_RESTORE_YY_MORE_OFFSET
 char *yytext;
 #line 1 "lexer.ll"
-/* Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+/* Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 
    Permission to use, copy, modify, and/or distribute this software for any
    purpose with or without fee is hereby granted, provided that the above

diff --git a/src/lib/eval/lexer.ll b/src/lib/eval/lexer.ll
index ff458cc0c6afa968e67bba6c80c331bca1702346..238c131356a208a15db0bd94b2c16178225b7f9b 100644 (file)
--- a/src/lib/eval/lexer.ll
+++ b/src/lib/eval/lexer.ll
@@ -1,4 +1,4 @@
-/* Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+/* Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 
    Permission to use, copy, modify, and/or distribute this software for any
    purpose with or without fee is hereby granted, provided that the above

diff --git a/src/lib/eval/parser.yy b/src/lib/eval/parser.yy
index 7ddedfe1a2d46aac1561172e065c89cf38f5596b..786a22546be7a59f98b5f7677bef201ef7aa51c6 100644 (file)
--- a/src/lib/eval/parser.yy
+++ b/src/lib/eval/parser.yy
@@ -1,4 +1,4 @@
-/* Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+/* Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 
    Permission to use, copy, modify, and/or distribute this software for any
    purpose with or without fee is hereby granted, provided that the above

diff --git a/src/lib/eval/tests/Makefile.am b/src/lib/eval/tests/Makefile.am
index 77d0ad2bf191be28d404bebff9be6c789dab0d9b..b284fb01e53fe7ebe15023736118346ebc9691da 100644 (file)
--- a/src/lib/eval/tests/Makefile.am
+++ b/src/lib/eval/tests/Makefile.am
@@ -26,7 +26,8 @@ if HAVE_GTEST
 
 TESTS += libeval_unittests
 
-libeval_unittests_SOURCES  = context_unittest.cc
+libeval_unittests_SOURCES  = boolean_unittest.cc
+libeval_unittests_SOURCES += context_unittest.cc
 libeval_unittests_SOURCES += evaluate_unittest.cc
 libeval_unittests_SOURCES += token_unittest.cc
 libeval_unittests_SOURCES += run_unittests.cc

diff --git a/src/lib/eval/tests/boolean_unittest.cc b/src/lib/eval/tests/boolean_unittest.cc
new file mode 100644 (file)
index 0000000..5e478ea
--- /dev/null
+++ b/src/lib/eval/tests/boolean_unittest.cc
@@ -0,0 +1,72 @@
+// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <config.h>
+#include <eval/eval_context.h>
+#include <eval/evaluate.h>
+#include <eval/token.h>
+#include <dhcp/pkt4.h>
+
+#include <boost/shared_ptr.hpp>
+#include <boost/scoped_ptr.hpp>
+#include <gtest/gtest.h>
+
+using namespace std;
+using namespace isc::dhcp;
+
+namespace {
+
+/// @brief Test fixture for testing booleans.
+class BooleanTest : public ::testing::Test {
+public:
+    void check(const string& expr, bool expected) {
+        EvalContext eval(Option::V4);
+        ASSERT_TRUE(eval.parseString(expr));
+        Pkt4Ptr pkt4(new Pkt4(DHCPDISCOVER, 12345));
+        if (expected) {
+            EXPECT_TRUE(evaluate(eval.expression, *pkt4));
+        } else {
+            EXPECT_FALSE(evaluate(eval.expression, *pkt4));
+        }
+    }
+};
+
+// A group of tests
+TEST_F(BooleanTest, tests) {
+    // true and (false or false)
+    check("('a' == 'a') and (('a' == 'b') or ('b' == 'a'))", false);
+    // (true and false) or false
+    check("(('a' == 'a') and ('a' == 'b')) or ('b' == 'a')", false);
+    // not true
+    check("not ('a' == 'a')", false);
+    // not false
+    check("not ('a' == 'b')", true);
+    // true and true and true and false
+    check("('a' == 'a') and ('b' == 'b') and ('c' == 'c') and ('a' == 'c')",
+          false);
+    // false or false or false or true
+    check("('a' == 'b') or ('a' == 'c') or ('b' == 'c') or ('b' == 'b')",
+          true);
+    // true or false or false or false
+    check("('a' == 'a') or ('a' == 'b') or ('a' == 'c') or ('b' == 'c')",
+          true);
+    // not (true or false)
+    check("not (('a' == 'a') or ('a' == 'b'))", false);
+    // not (true and false)
+    check("not (('a' == 'a') and ('a' == 'b'))", true);
+    // (not true) and false
+    check("(not ('a' == 'a')) and ('a' == 'b')",false);
+}
+
+};

diff --git a/src/lib/eval/tests/context_unittest.cc b/src/lib/eval/tests/context_unittest.cc
index d6f2f42659a9e06fc87d0922d2a01700362f1150..f7164a85bd4d9c365e5111ba2800fafd6fd46f65 100644 (file)
--- a/src/lib/eval/tests/context_unittest.cc
+++ b/src/lib/eval/tests/context_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

diff --git a/src/lib/eval/tests/evaluate_unittest.cc b/src/lib/eval/tests/evaluate_unittest.cc
index 0fde9bce82095f9192ac7780b6031185509c6c4e..0b575cad641edbbe445b4abacd73173d3f721afe 100644 (file)
--- a/src/lib/eval/tests/evaluate_unittest.cc
+++ b/src/lib/eval/tests/evaluate_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

diff --git a/src/lib/eval/tests/token_unittest.cc b/src/lib/eval/tests/token_unittest.cc
index 0fcf98f3fcf273b2811e0180b5ac18becb73302c..46d0471ed2d30a2e496a629906d81fd9b13fddc9 100644 (file)
--- a/src/lib/eval/tests/token_unittest.cc
+++ b/src/lib/eval/tests/token_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -106,6 +106,24 @@ public:
     /// @todo: Add more option types here
 };
 
+// This tests the toBool() conversions
+TEST_F(TokenTest, toBool) {
+
+    ASSERT_NO_THROW(Token::toBool("true"));
+    EXPECT_TRUE(Token::toBool("true"));
+    ASSERT_NO_THROW(Token::toBool("false"));
+    EXPECT_FALSE(Token::toBool("false"));
+
+    // Token::toBool() is case-sensitive
+    EXPECT_THROW(Token::toBool("True"), EvalTypeError);
+    EXPECT_THROW(Token::toBool("TRUE"), EvalTypeError);
+
+    // Proposed aliases
+    EXPECT_THROW(Token::toBool("1"), EvalTypeError);
+    EXPECT_THROW(Token::toBool("0"), EvalTypeError);
+    EXPECT_THROW(Token::toBool(""), EvalTypeError);
+}
+
 // This simple test checks that a TokenString, representing a constant string,
 // can be used in Pkt4 evaluation. (The actual packet is not used)
 TEST_F(TokenTest, string4) {

diff --git a/src/lib/eval/token.cc b/src/lib/eval/token.cc
index 289caa03d60e6786d284ca0a9891410b4ae3ca3e..f14ebe6428ff6f28c4428ade1eaf0d977169c77d 100644 (file)
--- a/src/lib/eval/token.cc
+++ b/src/lib/eval/token.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -190,14 +190,12 @@ TokenNot::evaluate(const Pkt& /*pkt*/, ValueStack& values) {
 
     string op = values.top();
     values.pop();
+    bool val = toBool(op);
 
-    if (op == "true") {
-        values.push("false");
-    } else if (op == "false") {
-        values.push("true");
+    if (val) {
+       values.push("false");
     } else {
-        isc_throw(EvalTypeError, "Expected a logical value at top of stack. "
-                  << "Got '" << op << "'.");
+       values.push("true");
     }
 }
 
@@ -211,28 +209,15 @@ TokenAnd::evaluate(const Pkt& /*pkt*/, ValueStack& values) {
 
     string op1 = values.top();
     values.pop();
+    bool val1 = toBool(op1);
     string op2 = values.top();
     values.pop(); // Dammit, std::stack interface is awkward.
+    bool val2 = toBool(op2);
 
-    if (op1 == "true") {
-        if (op2 == "true") {
-            values.push("true");
-        } else if (op2 == "false") {
-            values.push("false");
-        } else {
-            isc_throw(EvalTypeError, "Expected logical values at "
-                      << "top of stack. Got 'true' and '" << op2 << "'");
-        }
-    } else if (op1 == "false") {
-        if ((op2 == "true") || (op2 == "false")) {
-            values.push("false");
-        } else {
-            isc_throw(EvalTypeError, "Expected logical values at "
-                      << "top of stack. Got 'false' and '" << op2 << "'");
-        }
+    if (val1 && val2) {
+       values.push("true");
     } else {
-        isc_throw(EvalTypeError, "Expected logical values at top of stack. "
-                  << "Got '" << op1 << "' and '" << op2 << "'");
+       values.push("false");
     }
 }
 
@@ -246,28 +231,14 @@ TokenOr::evaluate(const Pkt& /*pkt*/, ValueStack& values) {
 
     string op1 = values.top();
     values.pop();
+    bool val1 = toBool(op1);
     string op2 = values.top();
     values.pop(); // Dammit, std::stack interface is awkward.
+    bool val2 = toBool(op2);
 
-    if (op1 == "true") {
-        if ((op2 == "true") || (op2 == "false")) {
-            values.push("true");
-        } else {
-            isc_throw(EvalTypeError, "Expected logical values at "
-                      << "top of stack. Got 'true' and '" << op2 << "'");
-        }
-    } else if (op1 == "false") {
-        if (op2 == "true") {
-            values.push("true");
-        } else if (op2 == "false") {
-            values.push("false");
-        } else {
-            isc_throw(EvalTypeError, "Expected logical values at "
-                      << "top of stack. Got 'false' and '" << op2 << "'");
-        }
+    if (val1 || val2) {
+       values.push("true");
     } else {
-        isc_throw(EvalTypeError, "Expected logical values at top of stack. "
-                  << "Got '" << op1 << "' and '" << op2 << "'");
+       values.push("false");
     }
 }
-

diff --git a/src/lib/eval/token.h b/src/lib/eval/token.h
index eacf51f905e8ed1832ee58e41749019f2871947a..073c4a40c9f2ee966654716fdf542a666eb34edf 100644 (file)
--- a/src/lib/eval/token.h
+++ b/src/lib/eval/token.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -87,6 +87,24 @@ public:
 
     /// @brief Virtual destructor
     virtual ~Token() {}
+
+    /// @brief Coverts a (string) value to a boolean
+    ///
+    /// Only "true" and "false" are expected.
+    ///
+    /// @param the (string) value
+    /// @return the boolean represented by the value
+    /// @throw EvalTypeError when the value is not either "true" or "false".
+    static inline bool toBool(std::string value) {
+        if (value == "true") {
+            return (true);
+        } else if (value == "false") {
+            return (false);
+        } else {
+            isc_throw(EvalTypeError, "Incorrect boolean. Expected exactly "
+                      "\"false\" or \"true\", got \"" << value << "\"");
+        }
+    }
 };
 
 /// @brief Token representing a constant string