Fix for Issue #10: Endless loop in BracketNotationParser bug

src/command_line/main.cc

@@ -62,16 +62,7 @@ int main(int argc, char** argv) {
         return -1;
     }
 
-    if (!bnp.validate_input(source_tree_string)) {
-        std::cerr << "Incorrect format of source tree. Is the number of opening and closing brackets equal?" << std::endl;
-        return -1;
-    }
     const node::Node<Label> source_tree = bnp.parse_single(source_tree_string);
-
-    if (!bnp.validate_input(dest_tree_string)) {
-        std::cerr << "Incorrect format of destination tree. Is the number of opening and closing brackets equal?" << std::endl;
-        return -1;
-    }
     const node::Node<Label> destination_tree = bnp.parse_single(dest_tree_string);
 
   std::cout << "Size of source tree:" << source_tree.get_tree_size() << std::endl;

src/label/json_label.h

@@ -42,6 +42,7 @@ namespace label {
 /// During the parsing process, the types keys and values cannot 
 class JSONLabel {
 public:
+  JSONLabel() = default;
   JSONLabel(const std::string& label);
 
   /// Operator overloadings.

src/label/string_label.h

@@ -49,6 +49,7 @@ namespace label {
 /// one wants to use a specialized cost model.
 class StringLabel {
 public:
+    StringLabel() = default;
     StringLabel(const std::string& label);
 
     /// Operator overloadings.

src/node/node.h

@@ -61,6 +61,7 @@ class Node {
 
 // Member functions
 public:
+  Node() = default;
   Node(ConstReference label);
 
   /// Returns the number of children of this node.

src/parser/bracket_notation_parser.h

@@ -86,14 +86,6 @@ class BracketNotationParser {
   /// \return Vector with all tokens.
   std::vector<std::string> get_tokens(const std::string& tree_string);
 
-  /// Validates the bracket notation input.
-  ///
-  /// NOTE: This function could be merged with parse_string but this may
-  ///       decrease readability.
-  ///
-  /// \param tree_string Tree in bracket notation.
-  /// \return True if the input is correct and false otherwise.
-  bool validate_input(const std::string& tree_string) const;
 // Member variables
 private:
   /// A stack to store nodes on a path to the root from the current node in the

src/parser/bracket_notation_parser_impl.h

@@ -32,42 +32,56 @@ template<class Label>
 node::Node<Label> BracketNotationParser<Label>::parse_single(
     const std::string& tree_string) {
 
+  // Tokenize the input string.
   std::vector<std::string> tokens = get_tokens(tree_string);
 
-  // Tokenize the input string - get iterator over tokens.
+  if (tree_string.size() < 2) {
+    throw std::invalid_argument("PARSER-ERROR: Tree string needs at least two characters.");
+  }
+
+  if (tokens[0] != "{") {
+    throw std::invalid_argument("PARSER-ERROR: First character must be an opening bracket.");
+  }
+
+  // Metadata for parser to verify the input.
+  int open_nodes = 1;
+
+  // Get iterator over input tokens.
   auto tokens_begin = tokens.begin();
   auto tokens_end = tokens.end();
 
   // Deal with the root node separately.
   ++tokens_begin; // Advance tokens to label.
   std::string match_str = *tokens_begin;
-  if (match_str == kLeftBracket || match_str == kRightBracket) { // Root has an empty label.
+  if (match_str == kLeftBracket) { // Root has an empty label.
     match_str = "";
     // Do not advance tokens - we're already at kLeftBracket or kRightBracket.
+  } else if (match_str == kRightBracket) { // Node has an empty label.
+    --open_nodes;
+    match_str = "";
+    ++tokens_begin; // Advance tokens.
   } else { // Non-empty label.
     ++tokens_begin; // Advance tokens.
   }
   Label root_label(match_str);
   node::Node<Label> root(root_label);
   node_stack.push_back(std::ref(root));
 
-  bool consumed_token = false;
-
   // Iterate all remaining tokens.
   while (tokens_begin != tokens_end) {
     match_str = *tokens_begin;
-    consumed_token = false;
 
     if (match_str == kLeftBracket) { // Enter node.
-      consumed_token = true;
-      ++tokens_begin; // Advance tokens to label.
-      match_str = *tokens_begin;
+      // ++tokens_begin; // Advance tokens to label.
+      match_str = *std::next(tokens_begin,1);
+      ++open_nodes;
 
       if (match_str == kLeftBracket || match_str == kRightBracket) { // Node has an empty label.
         match_str = "";
         // Do not advance tokens - we're already at kLeftBracket or kRightBracket.
       } else { // Non-empty label.
-        ++tokens_begin; // Advance tokens.
+        ++tokens_begin; // Advance tokens to get label.
+        match_str = *tokens_begin;
       }
 
       // Create new node.
@@ -79,18 +93,26 @@ node::Node<Label> BracketNotationParser<Label>::parse_single(
       // Put a reference to just-moved n (last child of its parent) on a stack.
       node_stack.push_back(std::ref(node_stack.back().get().add_child(n)));
     }
-
-    if (match_str == kRightBracket) { // Exit node.
-      consumed_token = true;
+    else if (match_str == kRightBracket) { // Exit node.
+      --open_nodes;
+      if (open_nodes < 0) {
+        throw std::invalid_argument("PARSER-ERROR: Found a closing bracket for missing opening bracket.");
+      }
       node_stack.pop_back();
-      ++tokens_begin; // Advance tokens.
     }
-
-    // Skip incorrect token.
-    if (consumed_token == false) {
-      ++tokens_begin; // Advance tokens.
+    else {
+      throw std::invalid_argument("PARSER-ERROR: Found unexpected token '" + match_str + "'.");
     }
+    ++tokens_begin; // Advance tokens.
   }
+
+  if (open_nodes > 0) {
+    throw std::invalid_argument("PARSER-ERROR: There are opening brackets that are never closed.");
+  }
+  if (open_nodes < 0) {
+    throw std::invalid_argument("PARSER-ERROR: Found a closing bracket for missing opening bracket.");
+  }
+
   return root;
 }
 
@@ -100,15 +122,17 @@ void BracketNotationParser<Label>::parse_collection(
     const std::string& file_path) {
   std::ifstream trees_file(file_path);
   if (!trees_file) {
-    throw std::runtime_error("ERROR: Problem with opening the file '" + file_path + "' in BracketNotationParser::parse_collection_efficient.");
+    throw std::invalid_argument("PARSER-ERROR: Problem with opening the file '" + file_path + "' in BracketNotationParser::parse_collection_efficient.");
   }
   // Read the trees line by line, parse, and move into the container.
   std::string tree_string;
   while (std::getline(trees_file, tree_string)) {
-    if (!validate_input(tree_string)) {
-      continue;
+    try {
+      trees_collection.push_back(parse_single(tree_string)); // -> This invokes a move constructor (due to push_back(<rvalue>)).
+    }
+    catch(const std::exception& e) {
+      std::cout << e.what();
     }
-    trees_collection.push_back(parse_single(tree_string)); // -> This invokes a move constructor (due to push_back(<rvalue>)).
   }
   trees_file.close();
 }
@@ -148,25 +172,4 @@ std::vector<std::string> BracketNotationParser<Label>::get_tokens(
   }
 
   return tokens;
-}
-
-template<class Label>
-bool BracketNotationParser<Label>::validate_input(const std::string& tree_string) const {
-  int bracket_diff_counter = 0; // Counts difference between the numbers of left and right brackets.
-  int bracket_pair_counter = 0; // Counts number of bracket pairs - number of nodes assuming correct nesting.
-  // Loop over all characters.
-  for(auto it = tree_string.begin(); it != tree_string.end(); ++it) {
-    if (*it == kEscapeChar) { // Skip next character if kEscapeChar is found.
-      ++it;
-    } else if (*it == kLeftBracket[0]) { // Increase bracket_counter when kLeftBracket found.
-      bracket_diff_counter++;
-      bracket_pair_counter++;
-    } else if (*it == kRightBracket[0]) { // Decrease bracket_counter when kRightBracket found.
-      bracket_diff_counter--;
-    }
-  }
-  if (bracket_diff_counter != 0 || bracket_pair_counter == 0) {
-    return false;
-  }
-  return true;
-}
+}

test/common/to_string_converters.h

@@ -200,3 +200,16 @@ const std::string map_to_string(const std::unordered_map<int, std::vector<int>>&
 }
 
 }
+
+/// Convert vector of int values to is string representation.
+///
+/// \param v Vector of int values.
+/// \return String representation of v.
+const std::string vector_to_string(const std::vector<std::string>& v) {
+  std::string s;
+  for (auto e : v) {
+    s += e + ",";
+  }
+  s.pop_back(); // Delete the last coma.s
+  return s;
+}

test/cted_ub/cted_ub_ted_test.cc

@@ -14,6 +14,13 @@ int main() {
   using Label = label::StringLabel;
   using CostModel = cost_model::UnitCostModelLD<Label>;
   using LabelDictionary = label::LabelDictionary<Label>;
+
+  // Declare parser.
+  parser::BracketNotationParser<Label> bnp;
+
+  // Trees.
+  node::Node<Label> t1;
+  node::Node<Label> t2;
 
   // Initialise label dictionary - separate dictionary for each test tree
   // becuse it is easier to keep track of label ids.
@@ -49,20 +56,15 @@ int main() {
       std::getline(test_cases_file, line);
       double correct_result = std::stod(line);
 
-      parser::BracketNotationParser<Label> bnp;
-
-      // Validate test trees.
-      if (!bnp.validate_input(input_tree_1_string)) {
-        std::cerr << "Incorrect format of source tree: '" << input_tree_1_string << "'. Is the number of opening and closing brackets equal?" << std::endl;
-        return -1;
+      // Parse test tree.
+      try {
+        t1 = bnp.parse_single(input_tree_1_string);
+        t2 = bnp.parse_single(input_tree_2_string);
       }
-      if (!bnp.validate_input(input_tree_2_string)) {
-        std::cerr << "Incorrect format of destination tree: '" << input_tree_2_string << "'. Is the number of opening and closing brackets equal?" << std::endl;
+      catch(const std::exception& e) {
+        std::cerr << e.what() << std::endl;
         return -1;
       }
-      // Parse test tree.
-      node::Node<Label> t1 = bnp.parse_single(input_tree_1_string);
-      node::Node<Label> t2 = bnp.parse_single(input_tree_2_string);
 
       // Index input trees.
       node::index_tree(ti1, t1, ld, ucm);

test/lgm_ub/lgm_ub_lb_fill_gaps_test.cc

@@ -15,6 +15,13 @@ int main() {
   using Label = label::StringLabel;
   using CostModel = cost_model::UnitCostModelLD<Label>;
   using LabelDictionary = label::LabelDictionary<Label>;
+
+  // Declare parser.
+  parser::BracketNotationParser<Label> bnp;
+
+  // Trees.
+  node::Node<Label> t1;
+  node::Node<Label> t2;
 
   // Initialise label dictionary - separate dictionary for each test tree
   // becuse it is easier to keep track of label ids.
@@ -38,8 +45,6 @@ int main() {
   node::TreeIndexLGM ti1;
   node::TreeIndexLGM ti2;
 
-  parser::BracketNotationParser<Label> bnp;
-
   // Fixed test cases.
   std::cout << "--- FIXED TEST ---" << std::endl;
 
@@ -60,20 +65,16 @@ int main() {
       std::getline(test_cases_file, line);
       std::string correct_result = line;
 
-      // Validate test trees.
-      if (!bnp.validate_input(input_tree_1_string)) {
-        std::cerr << "Incorrect format of source tree: '" << input_tree_1_string << "'. Is the number of opening and closing brackets equal?" << std::endl;
-        return -1;
+      // Parse test tree.
+      try {
+        t1 = bnp.parse_single(input_tree_1_string);
+        t2 = bnp.parse_single(input_tree_2_string);
       }
-      if (!bnp.validate_input(input_tree_2_string)) {
-        std::cerr << "Incorrect format of destination tree: '" << input_tree_2_string << "'. Is the number of opening and closing brackets equal?" << std::endl;
+      catch(const std::exception& e) {
+        std::cerr << e.what() << std::endl;
         return -1;
       }
 
-      // Parse test tree.
-      node::Node<Label> t1 = bnp.parse_single(input_tree_1_string);
-      node::Node<Label> t2 = bnp.parse_single(input_tree_2_string);
-
       // Index input trees.
       node::index_tree(ti1, t1, ld, ucm);
       node::index_tree(ti2, t2, ld, ucm);

test/lgm_ub/lgm_ub_lb_mapping_test.cc

@@ -15,6 +15,13 @@ int main() {
   using Label = label::StringLabel;
   using CostModel = cost_model::UnitCostModelLD<Label>;
   using LabelDictionary = label::LabelDictionary<Label>;
+
+  // Declare parser.
+  parser::BracketNotationParser<Label> bnp;
+
+  // Trees.
+  node::Node<Label> t1;
+  node::Node<Label> t2;
 
   // Initialise label dictionary - separate dictionary for each test tree
   // becuse it is easier to keep track of label ids.
@@ -38,8 +45,6 @@ int main() {
   node::TreeIndexLGM ti1;
   node::TreeIndexLGM ti2;
 
-  parser::BracketNotationParser<Label> bnp;
-
   // Fixed test cases.
   std::cout << "--- FIXED TEST ---" << std::endl;
 
@@ -60,20 +65,16 @@ int main() {
       std::getline(test_cases_file, line);
       std::string correct_result = line;
 
-      // Validate test trees.
-      if (!bnp.validate_input(input_tree_1_string)) {
-        std::cerr << "Incorrect format of source tree: '" << input_tree_1_string << "'. Is the number of opening and closing brackets equal?" << std::endl;
-        return -1;
+      // Parse test tree.
+      try {
+        t1 = bnp.parse_single(input_tree_1_string);
+        t2 = bnp.parse_single(input_tree_2_string);
       }
-      if (!bnp.validate_input(input_tree_2_string)) {
-        std::cerr << "Incorrect format of destination tree: '" << input_tree_2_string << "'. Is the number of opening and closing brackets equal?" << std::endl;
+      catch(const std::exception& e) {
+        std::cerr << e.what() << std::endl;
         return -1;
       }
 
-      // Parse test tree.
-      node::Node<Label> t1 = bnp.parse_single(input_tree_1_string);
-      node::Node<Label> t2 = bnp.parse_single(input_tree_2_string);
-
       // Index input trees.
       node::index_tree(ti1, t1, ld, ucm);
       node::index_tree(ti2, t2, ld, ucm);