From: Francis Dupont Date: Mon, 15 Feb 2016 15:47:27 +0000 (+0100) Subject: [4231] Addressed code comments X-Git-Tag: trac4267_base0~6^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=732af87d48ee1b4cb81f6abfadd7cfa3567e50bb;p=thirdparty%2Fkea.git [4231] Addressed code comments --- diff --git a/src/lib/eval/evaluate.cc b/src/lib/eval/evaluate.cc index a70ee2c6ed..dd5e0af68d 100644 --- 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 99843f2814..b9fbae02b5 100644 --- 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 ff458cc0c6..238c131356 100644 --- 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 7ddedfe1a2..786a22546b 100644 --- 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 77d0ad2bf1..b284fb01e5 100644 --- 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 index 0000000000..5e478ea0fb --- /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 +#include +#include +#include +#include + +#include +#include +#include + +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 d6f2f42659..f7164a85bd 100644 --- 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 0fde9bce82..0b575cad64 100644 --- 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 0fcf98f3fc..46d0471ed2 100644 --- 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 289caa03d6..f14ebe6428 100644 --- 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 eacf51f905..073c4a40c9 100644 --- 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