Apply Peter Schrammel's review comments

Author: martin

src/analyses/variable-sensitivity/abstract_enviroment.cpp

@@ -140,7 +140,7 @@ abstract_environmentt::eval(const exprt &expr, const namespacet &ns) const
 /// what kind of array abstraction it is).  So, as we find the variable
 /// ('a' in this case) we build a stack of which part of it is accessed.
 ///
-/// As abstractions may split the assignment into multiple write (for
+/// As abstractions may split the assignment into multiple writes (for
 /// example pointers that could point to several locations, arrays with
 /// non-constant indexes), each of which has to handle the rest of the
 /// compound write, thus the stack is passed (to write, which does the

src/analyses/variable-sensitivity/abstract_object.h

@@ -10,7 +10,7 @@
 /// abstract_objectt is the top of the inheritance heirarchy of objects
 /// used to represent individual variables in the general non-relational
 /// domain.  It is a two element abstraction (i.e. it is either top or
-/// bottom).  Within the hierachy of objects under it, child classes are
+/// bottom).  Within the hierarchy of objects under it, child classes are
 /// more precise abstractions (the converse doesn't hold to avoid
 /// diamonds and inheriting unnecessary fields).  Thus the common parent
 /// of two classes is an abstraction capable of representing both.  This

src/analyses/variable-sensitivity/array_abstract_object.cpp

@@ -131,9 +131,9 @@ abstract_object_pointert array_abstract_objectt::read_index(
 /// \param value: the value we are trying to assign to that value in the array
 /// \param merging_write: ?
 ///
-/// \return The struct_abstract_objectt representing the result of writing
+/// \return The array_abstract_objectt representing the result of writing
 ///          to a specific component. In this case this will always be top
-///          as we are not tracking the value of this struct.
+///          as we are not tracking the value in the array.
 ///
 /// A helper function to evaluate writing to a component of a struct.
 /// More precise abstractions may override this to

src/analyses/variable-sensitivity/constant_array_abstract_object.cpp

@@ -490,7 +490,7 @@ void constant_array_abstract_objectt::get_statistics(
   array_abstract_objectt::get_statistics(statistics, visited, env, ns);
   shared_array_mapt::viewt view;
   map.get_view(view);
-  for(auto const &object : view)
+  for(const auto &object : view)
   {
     if(visited.find(object.second) == visited.end())
     {

src/analyses/variable-sensitivity/data_dependency_context.cpp

@@ -210,7 +210,7 @@ abstract_object_pointert data_dependency_contextt::write(
  * \param update_sub_elements if true, propogate the update operation to any
  * children of this abstract object
  *
- * \return a clone of this abstract object with it's location context
+ * \return a clone of this abstract object with its location context
  * updated
  */
 abstract_object_pointert data_dependency_contextt::update_location_context(
@@ -280,7 +280,7 @@ data_dependency_contextt::merge(abstract_object_pointert other) const
 
 /**
  * Helper function for abstract_objectt::abstract_object_merge to perform any
- * additional actions after the base abstract_object_merge has completed it's
+ * additional actions after the base abstract_object_merge has completed its
  * actions but immediately prior to it returning. As such, this function gives
  * the ability to perform additional work for a merge.
  *

src/analyses/variable-sensitivity/full_struct_abstract_object.cpp

@@ -76,13 +76,12 @@ full_struct_abstract_objectt::full_struct_abstract_objectt(
   bool did_initialize_values = false;
   auto struct_type_it = struct_type_def.components().begin();
   for(auto param_it = e.operands().begin(); param_it != e.operands().end();
-      ++param_it)
+      ++param_it, ++struct_type_it)
   {
     map.insert_or_replace(
       struct_type_it->get_name(),
       environment.abstract_object_factory(param_it->type(), *param_it, ns));
     did_initialize_values = true;
-    ++struct_type_it;
   }
 
   if(did_initialize_values)

src/analyses/variable-sensitivity/three_way_merge_abstract_interpreter.cpp

@@ -132,7 +132,7 @@ bool ai_three_way_merget::visit_edge_function_call(
       ns);
 
     // TODO : this is probably needed to avoid three_way_merge modifying one of
-    // it's arguments as it goes.  A better solution would be to refactor
+    // its arguments as it goes.  A better solution would be to refactor
     // merge_three_way_function_return.
     const std::unique_ptr<statet> ptr_s_working_copy(
       make_temporary_state(s_working));
@@ -156,100 +156,3 @@ bool ai_three_way_merget::visit_edge_function_call(
   return new_data;
 }
 
-#if 0
-   // This is the edge from function end to return site.
- 
-   {
-     if(end_state.is_bottom())
-       return false; // function exit point not reachable
- 
-     working_sett working_set; // Redundant; visit will add l_return
- 
--    return visit_edge(
--      f_it->first, l_end, calling_function_id, l_return, ns, working_set);
-+    const std::unique_ptr<statet> tmp_state(make_temporary_state(end_state));
-+    tmp_state->transform(f_it->first, l_end, f_it->first, l_return, *this, ns);
-+
-+    const std::unique_ptr<statet> pre_merge_state{
-+      make_temporary_state(*tmp_state)};
-+
-+    const locationt l_begin = goto_function.body.instructions.begin();
-+    tmp_state->merge_three_way_function_return(
-+      get_state(l_call), get_state(l_begin), *pre_merge_state, ns);
-+
-+    return merge(*tmp_state, l_end, l_return);
-   }
-
-#endif
-
-#if 0
-bool ai_baset::visit_edge(
-  const irep_idt &function_id,
-  trace_ptrt p,
-  const irep_idt &to_function_id,
-  locationt to_l,
-  trace_ptrt caller_history,
-  const namespacet &ns,
-  working_sett &working_set)
-{
-  // We only care about the return cases
-  if (caller_history == ai_history_baset::no_caller_history) {
-    return ai_recursive_interproceduralt::visit_edge(function_id, p, to_function_id, to_l, caller_history, ns, working_set);
-  }
-
-  // There are four histories / locations / domains we care about
-  // In chronological order...
-
-  trace_ptr call_site_history = caller_history;
-  statet &call_site_state = get_state(call_site_history);
-  locationt call_site_location = call_site_history.current_location();
-  INVARIANT(call_site_location->is_function_call(), "caller_history implies that is is a function call");
-
-  trace_ptr callee_start_history = TBD;
-  statet &callee_start_state = get_state(callee_start_history);
-  locationt callee_start_location = callee_start_history.current_location();
-
-  trace_ptr callee_end_history = p;
-  statet &callee_end_state = get_state(callee_end_history);
-  locationt callee_end_location = callee_end_history.current_location();
-  INVARIANT(callee_end_location->is_end_function(), "TBD");
-
-  trace_ptr return_site_history = STEP;
-  statet &return_site_state = get_state(return_site_history);
-  locationt return_site_location = to_l;
-  INVARIANT(std::next(call_site_location) == to_l, "TBD");
-
-
-  
-  // Has history taught us not to step here...
-  auto next =
-    p->step(to_l, *(storage->abstract_traces_before(to_l)), caller_history);
-  if(next.first == ai_history_baset::step_statust::BLOCKED)
-    return false;
-  trace_ptrt to_p = next.second;
-
-  // Abstract domains are mutable so we must copy before we transform
-  statet &current = get_state(p);
-
-  std::unique_ptr<statet> tmp_state(make_temporary_state(current));
-  statet &new_values = *tmp_state;
-
-  // Apply transformer
-  new_values.transform(function_id, p, to_function_id, to_p, *this, ns);
-
-  // Expanding a domain means that it has to be analysed again
-  // Likewise if the history insists that it is a new trace
-  // (assuming it is actually reachable).
-  if(
-    merge(new_values, p, to_p) ||
-    (next.first == ai_history_baset::step_statust::NEW &&
-     !new_values.is_bottom()))
-  {
-    put_in_working_set(working_set, to_p);
-    return true;
-  }
-
-  return false;
-}
-
-#endif

src/analyses/variable-sensitivity/variable_sensitivity_domain.h

@@ -8,14 +8,15 @@
 
 /// \file
 /// There are different ways of handling arrays, structures, unions and
-/// pointers.  Our existing solution basically ignores them which is
-/// imprecise at best and out-right wrong at worst.  For one project we
-/// needed to do better.  We could have implemented a particular way of
-/// handling them in an existing domain, created a new one with it, etc.
-/// This would work but it means duplicate code and it is is inflexible when
-/// the same person / the next person comes along and says "actually, we
-/// really care about the pointer precision but less so the array so could
-/// you just ...".  Thus the idea was to do this properly:
+/// pointers.  interval_domaint and constant_propagator_domaint
+/// basically ignores them which is imprecise at best and out-right
+/// wrong at worst.  For one project we needed to do better.  We could
+/// have implemented a particular way of handling them in an existing
+/// domain, created a new one with it, etc.  This would work but it
+/// means duplicate code and it is is inflexible when the same person
+/// / the next person comes along and says "actually, we really care
+/// about the pointer precision but less so the array so could you
+/// just ...".  Thus the idea was to do this properly:
 ///
 /// 1. Build a "non-relational domain" and allow the abstractions used for
 /// individual variables to be different.

src/analyses/variable-sensitivity/write_location_context.cpp

@@ -15,13 +15,13 @@
 
 /**
  * Update the location context for an abstract object, potentially
- * propogating the update to any children of this abstract object.
+ * propagating the update to any children of this abstract object.
  *
  * \param locations the set of locations to be updated
  * \param update_sub_elements if true, propogate the update operation to any
  * children of this abstract object
  *
- * \return a clone of this abstract object with it's location context
+ * \return a clone of this abstract object with its location context
  * updated
  */
 abstract_object_pointert write_location_contextt::update_location_context(
@@ -152,7 +152,7 @@ write_location_contextt::merge(abstract_object_pointert other) const
 
 /**
  * Helper function for abstract_objectt::abstract_object_merge to perform any
- * additional actions after the base abstract_object_merge has completed it's
+ * additional actions after the base abstract_object_merge has completed its
  * actions but immediately prior to it returning. As such, this function gives
  * the ability to perform additional work for a merge.
  *

src/analyses/variable-sensitivity/write_stack.cpp

@@ -2,7 +2,7 @@
 
  Module: Variable Sensitivity Domain
 
- Author: DiffBlue Limited. All rights reserved.
+ Author: DiffBlue Limited.
 
 \*******************************************************************/