[4483] Review comments addressed

 - Implemented new test for checking integer boundaries
 - checkTokenInteger documented
 - indentation fixed in parser.yy
 - integer check in lexer.ll extended from int to int64_t
 - a bug fixed in EvalContext::convertUint32
 - User's Guide now explicitly mentions network order

doc/guide/classify.xml

@@ -433,7 +433,9 @@
       it is easier to use decimal notation to represent integers, but it is also
       possible to use hex notation. When using hex notation to represent an
       integer care should be taken to make sure the value is represented as 32
-      bits, e.g. use 0x00000001 instead of 0x1 or 0x01.
+      bits, e.g. use 0x00000001 instead of 0x1 or 0x01. Also, make
+      sure the value is specified in network order, e.g. 1 is
+      represented as 0x00000001.
       </para>
 
       <para>

src/lib/eval/eval_context.cc

@@ -139,7 +139,7 @@ EvalContext::convertUint32(const std::string& number,
     } catch (const boost::bad_lexical_cast &) {
         error(loc, "Invalid value in " + number);
     }
-    if (n >= std::numeric_limits<uint32_t>::max()) {
+    if (n > std::numeric_limits<uint32_t>::max()) {
         error(loc, "Invalid value in "
               + number + ". Allowed range: 0..4294967295");
     }

src/lib/eval/lexer.ll

@@ -110,7 +110,11 @@ addr6 [0-9a-fA-F]*\:[0-9a-fA-F]*\:[0-9a-fA-F:.]*
     std::string tmp(yytext);
 
     try {
-        static_cast<void>(boost::lexical_cast<int>(tmp));
+        // In substring we want to use negative values (e.g. -1).
+        // In enterprise-id we need to use values up to 0xffffffff.
+        // To cover both of those use cases, we need at least
+        // int64_t.
+        static_cast<void>(boost::lexical_cast<int64_t>(tmp));
     } catch (const boost::bad_lexical_cast &) {
         driver.error(loc, "Failed to convert " + tmp + " to an integer.");
     }

src/lib/eval/parser.yy

@@ -371,18 +371,18 @@ string_expr : STRING
                                                                TokenVendor::DATA, index));
                     ctx.expression.push_back(vendor_class);
                 }
-             | integer_expr
+            | integer_expr
                 {
                     TokenPtr integer(new TokenInteger($1));
                     ctx.expression.push_back(integer);
                 }
             ;
 
-integer_expr: INTEGER
-              {
-                  $$ = ctx.convertUint32($1, @1);
-              }
-            ;
+integer_expr : INTEGER
+                 {
+                     $$ = ctx.convertUint32($1, @1);
+                 }
+             ;
 
 option_code : INTEGER
                  {

src/lib/eval/tests/context_unittest.cc

@@ -96,6 +96,10 @@ public:
         EXPECT_TRUE(eq);
     }
 
+    /// @brief Checks if the given token is integer with expected value
+    ///
+    /// @param token token to be inspected
+    /// @param exp_value expected integer value of the token
     void checkTokenInteger(const TokenPtr& token, uint32_t exp_value) {
         ASSERT_TRUE(token);
         boost::shared_ptr<TokenInteger> integer =

src/lib/eval/tests/evaluate_unittest.cc

@@ -314,7 +314,8 @@ public:
         bool result;
         bool parsed;
 
-        EXPECT_NO_THROW(parsed = eval.parseString(expr));
+        EXPECT_NO_THROW(parsed = eval.parseString(expr))
+            << " while parsing expression " << expr;
         EXPECT_TRUE(parsed) << " for expression " << expr;
 
         switch (u) {
@@ -330,6 +331,19 @@ public:
 
         EXPECT_EQ(exp_result, result) << " for expression " << expr;
     }
+
+    /// @brief Checks that specified expression throws expected exception.
+    ///
+    /// @tparam ex exception type expected to be thrown
+    /// @param expr expression to be evaluated
+    template<typename ex>
+    void testExpressionNegative(const std::string& expr,
+                                const Option::Universe& u = Option::V4) {
+        EvalContext eval(u);
+
+        EXPECT_THROW(eval.parseString(expr), ex) << "while parsing expression "
+                                                 << expr;
+    }
 };
 
 // This is a quick way to check if certain expressions are valid or not and
@@ -402,6 +416,8 @@ TEST_F(ExpressionsTest, expressionsPkt4Hlen) {
     testExpression(Option::V4, "pkt4.mac == 0x010203040506", true);
 }
 
+// Test if expressions message type can be detected in Pkt4.
+// It also doubles as a check for integer comparison here.
 TEST_F(ExpressionsTest, expressionsPkt4type) {
 
     // We can inspect the option content directly, but
@@ -416,4 +432,17 @@ TEST_F(ExpressionsTest, expressionsPkt4type) {
     testExpression(Option::V4, "pkt4.msgtype == 2", false);
 }
 
+// This tests if inappropriate values (negative, too large) are
+// rejected, but extremal value still allowed for uint32_t are ok.
+TEST_F(ExpressionsTest, invalidIntegers) {
+
+    // These are the extremal uint32_t values that still should be accepted.
+    testExpression(Option::V4, "4294967295 == 0", false);
+
+    // Negative integers should be rejected.
+    testExpressionNegative<EvalParseError>("4294967295 == -1");
+
+    // Oops, one too much.
+    testExpressionNegative<EvalParseError>("4294967296 == 0");
+}
 };