[trac4090] Update per review comments.

Update per the review comments except for the logging
stuff which will be done next.

ChangeLog

@@ -1,3 +1,7 @@
+XXXX.	[func]		sar
+	Added client classification substring token.
+	(Trac #4090, git <tbd>
+
 1041.	[func]		tomek
 	A new library, libkea-eval has been edded. It is not functional
 	yet, but its purpose is to provide a generic expression

src/lib/eval/eval_messages.mes

@@ -21,4 +21,5 @@ client classification expressions.
 
 % EVAL_SUBSTRING_BAD_PARAM_CONVERSION starting %1, length %2
 This debug message indicates that the parameter for the starting postion
-or length of the substring couldn't be converted to an integer.
+or length of the substring couldn't be converted to an integer.  In this
+case the substring routine returns an empty string.

src/lib/eval/tests/token_unittest.cc

@@ -233,7 +233,7 @@ TEST_F(TokenTest, optionEqualTrue) {
 };
 
 // This test checks if an a token representing a substring request
-// throws an excpetion if there aren't enough values on the stack.
+// throws an exception if there aren't enough values on the stack.
 // The stack from the top is: length, start, string.
 // The actual packet is not used.
 TEST_F(TokenTest, substringNotEnoughValues) {
@@ -340,8 +340,9 @@ TEST_F(TokenTest, substringBadParams) {
     verifySubstringEval("foobar", "0", "allaboard", "");
 }
 
-// lastly check that we don't get anything if the string is empty
-TEST_F(TokenTest, substringStartingEmpty) {
+// lastly check that we don't get anything if the string is empty or
+// we don't ask for any characters from it.
+TEST_F(TokenTest, substringReturnEmpty) {
     verifySubstringEval("", "0", "all", "");
     verifySubstringEval("foobar", "0", "0", "");
 }
@@ -352,7 +353,7 @@ TEST_F(TokenTest, substringStartingEmpty) {
 // result on the bottom with the substring result on next.
 // Evaulating the equals should produce true for the first
 // and false for the second.
-// throws an excpetion if there aren't enough values on the stack.
+// throws an exception if there aren't enough values on the stack.
 // The stack from the top is: length, start, string.
 // The actual packet is not used.
 TEST_F(TokenTest, substringEquals) {

src/lib/eval/token.cc

@@ -76,14 +76,15 @@ TokenSubstring::evaluate(const Pkt& /*pkt*/, ValueStack& values) {
         return;
     }
 
-    // Convert the starting position and legnth from strings to numbers
+    // Convert the starting position and length from strings to numbers
     // the length may also be "all" in which case simply make it the
-    // legnth of the string.
+    // length of the string.
     // If we have a problem push an empty string and leave
-    int start_pos, length;
+    int start_pos;
+    int length;
     try {
         start_pos = boost::lexical_cast<int>(start_str);
-        if ("all" == len_str) {
+        if (len_str == "all") {
             length = string_str.length();
         } else {
             length = boost::lexical_cast<int>(len_str);
@@ -100,19 +101,19 @@ TokenSubstring::evaluate(const Pkt& /*pkt*/, ValueStack& values) {
         return;
     }
 
+    const int string_length = string_str.length();
     // If the starting postion is outside of the string push an
     // empty string and leave
-    if (((start_pos >= 0) && (start_pos >= string_str.length())) |
-        ((start_pos < 0) && (-start_pos > string_str.length()))) {
+    if ((start_pos < -string_length) || (start_pos >= string_length)) {
         values.push("");
         return;
     }
 
     // Adjust the values to be something for substr.  We first figure out
-    // the staring postion, then update it and the length to get the
-    // characters before or after it depnding on the sign of length
+    // the starting postion, then update it and the length to get the
+    // characters before or after it depending on the sign of length
     if (start_pos < 0) {
-        start_pos = string_str.length() + start_pos;
+        start_pos = string_length + start_pos;
     }
 
     if (length < 0) {

src/lib/eval/token.h

@@ -150,10 +150,10 @@ public:
     /// either "true" or "false". It requires at least two parameters to be
     /// present on stack.
     ///
-    /// @throw EvalBadStack if there's less than 2 values on stack
+    /// @throw EvalBadStack if there are less than 2 values on stack
     ///
-    /// @brief pkt (unused)
-    /// @brief values - stack of values (2 arguments will be poped, 1 result
+    /// @param pkt (unused)
+    /// @param values - stack of values (2 arguments will be popped, 1 result
     ///        will be pushed)
     void evaluate(const Pkt& pkt, ValueStack& values);
 };
@@ -170,36 +170,45 @@ public:
 
     /// @brief Extract a substring from a string
     ///
-    /// Evaluation does not use packet information, but rather consumes the last
-    /// three parameters and requires at least three parameters to be present on
-    /// stack.  From the top it expects the values on the stack as:
-    ///  len
-    ///  start
-    ///  str
+    /// Evaluation does not use packet information.  It requires at least
+    /// three values to be present on the stack.  It will consume the top
+    /// three values on the stack as parameters and push the resulting substring
+    /// onto the stack.  From the top it expects the values on the stack as:
+    /// -  len
+    /// -  start
+    /// -  str
     ///
-    /// str is the string to extract a substring from.  If it is empty an empty
+    /// str is the string to extract a substring from.  If it is empty, an empty
     /// string is pushed onto the value stack.
     ///
     /// start is the postion from which the code starts extracting the substring.
-    /// 0 is before the first character and a negative number starts from the end,
-    /// with -1 being before the last character.  If the starting point is
-    /// outside of the original string an empty string is pushed onto the value
-    /// stack.
+    /// 0 is the first character and a negative number starts from the end, with
+    /// -1 being the last character.  If the starting point is outside of the
+    /// original string an empty string is pushed onto the value stack.
     ///
     /// length is the number of characters from the string to extract.
     /// "all" means all remaining characters from start to the end of string.
     /// A negative number means to go from start towards the beginning of
-    /// string, but doesn't include start.
+    /// the string, but doesn't include start.
     /// If length is longer than the remaining portion of string
     /// then the entire remaining portion is placed on the value stack.
-    /// Note: a negative value of length only indicates which characters to
-    /// extract, it does NOT indicate any attempt to reverse the string.
-    /// For example substring("foobar", 4, -2) will result in "ob".
-    ///
-    /// @throw EvalBadStack if there's less than 3 values on stack
     ///
-    /// @brief pkt (unused)
-    /// @brief values - stack of values (3 arguments will be poped, 1 result
+    /// The following examples all use the base string "foobar", the first number
+    /// is the starting position and the second is the length.  Note that
+    /// a negative length only selects which characters to extract it does not
+    /// indicate an attempt to reverse the string.
+    /// -  0, all => "foobar"
+    /// -  0,  6  => "foobar"
+    /// -  0,  4  => "foob"
+    /// -  2, all => "obar"
+    /// -  2,  6  => "obar"
+    /// - -1, all => "r"
+    /// - -1, -4  => "ooba"
+    ///
+    /// @throw EvalBadStack if there are less than 3 values on stack
+    ///
+    /// @param pkt (unused)
+    /// @param values - stack of values (3 arguments will be popped, 1 result
     ///        will be pushed)
     void evaluate(const Pkt& pkt, ValueStack& values);
 };