Some refactoring in convert_instructions

This extract two methods from the main function, and makes some simple
improvements in the code:
use auto for iterator types, use `at` instead of find+assert.

Author: romainbrenguier

src/java_bytecode/java_bytecode_convert_method.cpp

@@ -975,10 +975,7 @@ codet java_bytecode_convert_methodt::convert_instructions(
   std::vector<unsigned> jsr_ret_targets;
   std::vector<instructionst::const_iterator> ret_instructions;
 
-  for(instructionst::const_iterator
-      i_it=instructions.begin();
-      i_it!=instructions.end();
-      i_it++)
+  for(auto i_it = instructions.begin(); i_it != instructions.end(); i_it++)
   {
     converted_instructiont ins=converted_instructiont(i_it, code_skipt());
     std::pair<address_mapt::iterator, bool> a_entry=
@@ -1018,18 +1015,7 @@ codet java_bytecode_convert_methodt::convert_instructions(
        i_it->statement=="invokespecial" ||
        i_it->statement=="invokeinterface")
     {
-      // find the corresponding try-catch blocks and add the handlers
-      // to the targets
-      for(const auto &exception_row : method.exception_table)
-      {
-        if(i_it->address>=exception_row.start_pc &&
-           i_it->address<exception_row.end_pc)
-        {
-          a_entry.first->second.successors.push_back(
-            exception_row.handler_pc);
-          targets.insert(exception_row.handler_pc);
-        }
-      }
+      add_try_catch_handler_to_target(method, targets, i_it, a_entry);
     }
 
     if(i_it->statement=="goto" ||
@@ -1052,10 +1038,9 @@ codet java_bytecode_convert_methodt::convert_instructions(
       if(i_it->statement=="jsr" ||
          i_it->statement=="jsr_w")
       {
-        instructionst::const_iterator next=i_it+1;
-        assert(
-          next!=instructions.end() &&
-          "jsr without valid return address?");
+        auto next = i_it + 1;
+        DATA_INVARIANT(
+          next != instructions.end(), "jsr should have valid return address");
         targets.insert(next->address);
         jsr_ret_targets.push_back(next->address);
       }
@@ -1083,27 +1068,12 @@ codet java_bytecode_convert_methodt::convert_instructions(
       ret_instructions.push_back(i_it);
     }
   }
-
-  // Draw edges from every `ret` to every `jsr` successor. Could do better with
-  // flow analysis to distinguish multiple subroutines within the same function.
-  for(const auto retinst : ret_instructions)
-  {
-    auto &a_entry=address_map.at(retinst->address);
-    a_entry.successors.insert(
-      a_entry.successors.end(),
-      jsr_ret_targets.begin(),
-      jsr_ret_targets.end());
-  }
+  draw_edges_from_ret_to_jsr(address_map, jsr_ret_targets, ret_instructions);
 
   for(const auto &address : address_map)
   {
     for(unsigned s : address.second.successors)
-    {
-      address_mapt::iterator a_it=address_map.find(s);
-      assert(a_it!=address_map.end());
-
-      a_it->second.predecessors.insert(address.first);
-    }
+      address_map.at(s).predecessors.insert(address.first);
   }
 
   // Clean the list of temporary variables created by a call to `tmp_variable`.
@@ -1123,28 +1093,23 @@ codet java_bytecode_convert_methodt::convert_instructions(
 
   while(!working_set.empty())
   {
-    std::set<unsigned>::iterator cur=working_set.begin();
-    address_mapt::iterator a_it=address_map.find(*cur);
-    CHECK_RETURN(a_it != address_map.end());
+    auto cur = working_set.begin();
+    auto a = address_map.at(*cur);
     unsigned cur_pc=*cur;
     working_set.erase(cur);
 
-    if(a_it->second.done)
+    if(a.done)
       continue;
-    working_set
-      .insert(a_it->second.successors.begin(), a_it->second.successors.end());
+    working_set.insert(a.successors.begin(), a.successors.end());
 
-    instructionst::const_iterator i_it=a_it->second.source;
-    stack.swap(a_it->second.stack);
-    a_it->second.stack.clear();
-    codet &c=a_it->second.code;
+    instructionst::const_iterator i_it = a.source;
+    stack.swap(a.stack);
+    a.stack.clear();
+    codet &c = a.code;
 
     assert(
-      stack.empty() ||
-      a_it->second.predecessors.size()<=1 ||
-      has_prefix(
-        stack.front().get_string(ID_C_base_name),
-        "$stack"));
+      stack.empty() || a.predecessors.size() <= 1 ||
+      has_prefix(stack.front().get_string(ID_C_base_name), "$stack"));
 
     irep_idt statement=i_it->statement;
     exprt arg0=i_it->args.size()>=1?i_it->args[0]:nil_exprt();
@@ -2145,8 +2110,8 @@ codet java_bytecode_convert_methodt::convert_instructions(
       if(clinit_call.get_statement()!=ID_skip)
       {
         code_blockt ret_block;
-        ret_block.move_to_operands(clinit_call);
-        ret_block.move_to_operands(c);
+        ret_block.add(clinit_call);
+        ret_block.add(c);
         c=std::move(ret_block);
       }
       results[0]=tmp;
@@ -2490,8 +2455,8 @@ codet java_bytecode_convert_methodt::convert_instructions(
 
     push(results);
 
-    a_it->second.done=true;
-    for(const unsigned address : a_it->second.successors)
+    a.done = true;
+    for(const unsigned address : a.successors)
     {
       address_mapt::iterator a_it2=address_map.find(address);
       CHECK_RETURN(a_it2 != address_map.end());
@@ -2711,6 +2676,45 @@ codet java_bytecode_convert_methodt::convert_instructions(
   return code;
 }
 
+void java_bytecode_convert_methodt::draw_edges_from_ret_to_jsr(
+  java_bytecode_convert_methodt::address_mapt &address_map,
+  std::vector<unsigned int> &jsr_ret_targets,
+  const std::vector<
+    std::vector<java_bytecode_parse_treet::instructiont>::const_iterator>
+    &ret_instructions) const
+{ // Draw edges from every `ret` to every `jsr` successor. Could do better with
+  // flow analysis to distinguish multiple subroutines within the same function.
+  for(const auto retinst : ret_instructions)
+  {
+    auto &a_entry = address_map.at(retinst->address);
+    a_entry.successors.insert(
+      a_entry.successors.end(), jsr_ret_targets.begin(), jsr_ret_targets.end());
+  }
+}
+
+void java_bytecode_convert_methodt::add_try_catch_handler_to_target(
+  const java_bytecode_convert_methodt::methodt &method,
+  std::set<unsigned int> &targets,
+  const std::vector<java_bytecode_parse_treet::instructiont>::const_iterator
+    &i_it,
+  const std::pair<
+    std::map<unsigned int,
+             java_bytecode_convert_methodt::converted_instructiont>::iterator,
+    bool> &a_entry) const
+{ // find the corresponding try-catch blocks and add the handlers
+  // to the targets
+  for(const auto &exception_row : method.exception_table)
+  {
+    if(
+      i_it->address >= exception_row.start_pc &&
+      i_it->address < exception_row.end_pc)
+    {
+      a_entry.first->second.successors.push_back(exception_row.handler_pc);
+      targets.insert(exception_row.handler_pc);
+    }
+  }
+}
+
 /// This uses a cut-down version of the logic in
 /// java_bytecode_convert_methodt::convert to initialize symbols for the
 /// parameters and update the parameters in the type of method_symbol with

src/java_bytecode/java_bytecode_convert_method_class.h

@@ -271,6 +271,23 @@ class java_bytecode_convert_methodt:public messaget
     const typet &,
     code_blockt &,
     exprt &);
+
+  void add_try_catch_handler_to_target(
+    const methodt &method,
+    std::set<unsigned int> &targets,
+    const std::vector<java_bytecode_parse_treet::instructiont>::const_iterator
+      &i_it,
+    const std::pair<
+      std::map<unsigned int,
+               java_bytecode_convert_methodt::converted_instructiont>::iterator,
+      bool> &a_entry) const;
+
+  void draw_edges_from_ret_to_jsr(
+    address_mapt &address_map,
+    std::vector<unsigned int> &jsr_ret_targets,
+    const std::vector<
+      std::vector<java_bytecode_parse_treet::instructiont>::const_iterator>
+      &ret_instructions) const;
 };
 
 #endif